Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Meditor 910 history reporting incorrectly #66

Merged

Conversation

aritrivi
Copy link
Contributor

Fix null modifiedOn date for initial document state. Improve history detail layout in the history panel for readability. History issues are still a potential problem when users "edit"/"save"/"submit for review" from a browser window that is not refreshed and current with any new status changes.

@joncarlson
Copy link
Collaborator

Was confused for a second before realizing this is merging from a fork. For anyone else reviewing, here's what I did to use my existing mEditor folder:

git remote add aritrivi https://github.com/aritrivi/gesdisc-meditor.git
git fetch aritrivi
git checkout meditor-910-History-reporting-incorrectly

Just make sure to switch back to your origin branch when you're done

@@ -95,11 +95,12 @@ export async function createDocument(
}

//* This logic (and associated TODO) is ported from Meditor.js, saveDocument. Minimal modifications were made.
const rootState = { source: INIT_STATE, target: initialState }
const modDate = new Date().toISOString()
Copy link
Collaborator

@joncarlson joncarlson Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit, took me a bit to figure out what modDate was, may be better spelled out, like modifiedOn

Copy link
Collaborator

@joncarlson joncarlson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Much easier to see the transitions between states. Going ahead and approving, had a small comment

Copy link
Contributor

@benforshey benforshey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The history panel looks good, Tony! The toLocaleString call is a nice touch.

It looks like Prettier didn't run on these files, which means that the next time someone makes a change and Prettier does run, it'll add your files to their PR for just formatting changes. You can address that by checking which files aren't formatted correctly:

  • From packages/app in your terminal, run npx prettier -l . This gives you the list of files that don't match our Prettier config.
  • For each file, feed those files into Prettier with npx prettier -w <your-file.extension> or just YOLO it once with npx prettier -w . (this is actually pretty safe since you can just discard changes if any files change that are unrelated to your PR).

aritrivi and others added 2 commits November 20, 2024 12:46
@aritrivi
Copy link
Contributor Author

All PR comments addressed.

@aritrivi aritrivi closed this Nov 20, 2024
@aritrivi aritrivi reopened this Nov 20, 2024
@benforshey benforshey merged commit cb44d9e into nasa:main Nov 20, 2024
0 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants