NASA-AMMOS / aerie-ui

The client application for Aerie.
https://nasa-ammos.github.io/aerie-docs/
MIT License
28 stars 4 forks source link

Sequence Editor Followup #1291

Open duranb opened 1 month ago

duranb commented 1 month ago

The following are some things that that we found from the sequence editor PR that were left out of the review in order to quickly push it through:

### Tasks
- [ ] `DictionaryTable.svelte` - maintaining two different selections based multiselect can potentially become difficult to maintain consistent interactions. Refactor so that potentially only one list of selected IDs is maintained - `IN PROGRESS`
- [ ] https://github.com/NASA-AMMOS/aerie-ui/issues/1292
- [x] `ArgumentTooltip.svelte` - Confirm that for displays like `arg.bit_length ?? 'None'` the intent is to show `0` instead of `None` when the value is `0`
- [x] `ArgTitle.svelte` - title should be set to variable that is set in a reactive statement to prevent unnecessary re-renders - https://github.com/NASA-AMMOS/aerie-ui/pull/1304
- [ ] Expansion/Parcel/Sequence update queries should be combined into one
- [x] `NumEditor.svelte` - input element should be of type `number` instead of `string`. - `IN PROGRESS`
- [x] `NumEditor.svelte` - cleanup unused code - `IN PROGRESS`
- [ ] `SelectedCommand.svelte` - address TODOs
- [x] Repeat arg hovering doesn't trigger a tooltip in editor - `IN PROGRESS`
- [x] Remove `utils.ts` as that was renamed to `codemirror-utils.ts` - https://github.com/NASA-AMMOS/aerie-ui/pull/1324
- [ ] Add unit tests for all the new utility TS files under `/codemirror` and `/new-sequence-editor`
- [x] `dictionaries/+page.svelte` - the `'Adaptation'` case can be changed to use the enum representation - `IN PROGRESS`
- [ ] Is it necessary to keep `logger.ts`? Shouldn't those errors/messages be more exposed?
- [ ] Address any TODOs in `/new-sequence-editor` if addressable
- [ ] Move `sequencer-grammar-constants.ts` to a new `/constants` directory for easier access across project
- [x] `time-utils.ts` - I don't think that the `g` flag is required for each of the regular expressions. Once those are removed, setting the `regex.lastIndex` would no longer be necessary. - https://github.com/NASA-AMMOS/aerie-ui/pull/1324
- [x] `time-utils.ts` - For `isTimeBalanced`, rather than passing the regex, can we just pass in defined type constants or enums (i.e. 'ABSOLUTE_TIME') so that the arguments are much more explicit - https://github.com/NASA-AMMOS/aerie-ui/pull/1324
- [x] https://github.com/NASA-AMMOS/aerie-ui/issues/1318
- [x] `grammar.test.ts` - update to be more understandable and less fragile @joswig? - https://github.com/NASA-AMMOS/aerie-ui/pull/1303
- [ ] Delete `aerie-phoenix-wordmark.svg` if not being used @joswig - `IN PROGRESS`
- [ ] `SearchableDropdown.svelte` - @joswig I think that displaying a message somewhere on the list (bottom maybe?) would be necessary if the list is truncated to avoid confusion
- [ ] `permissions.ts` - Rename permission for `commandDictionary` to generic `dictionary` if it's no longer command dictionary specific. (`DELETE_COMMAND_DICTIONARY` should be renamed as well if that's no longer relevant) - `IN PROGRESS`
- [ ] Prevent clicking on download/copy buttons when nothing is selected - `IN PROGRESS`
- [ ] Vertically align the "filter to my sequences" checkbox and text on the sequencing page - `IN PROGRESS`
- [ ] Style the editor tooltip for better legibility - `IN PROGRESS`
- [ ] Style the Selected Command section with proper header and Aerie components - `IN PROGRESS`
- [ ] Discuss why the sequence editor has its own branding
- [ ] Empty state for third column in sequence editor - `IN PROGRESS`
- [ ] Document usage of `globalThis` - https://github.com/NASA-AMMOS/aerie-docs/pull/162
- [x] De-dupe and comment `isTimeBalanced` which can be found in both `time-utils.ts` and `timeDiagnostics.ts` - https://github.com/NASA-AMMOS/aerie-ui/pull/1324
- [x] More docstrings for `time-utils.ts` functions - https://github.com/NASA-AMMOS/aerie-ui/pull/1324
- [ ] Move the new dependencies from `devDependencies` to `dependencies` in package.json
- [ ] Uploading an invalid file type results in a silent failure on Dictionary form
- [ ] Able to uncheck a dictionary (and not select a new one) from a parcel, but saving doesn't persist. - `IN PROGRESS`
- [ ] Add tests for adding / removing dictionaries to parcels
- [x] Unify time formats across eDSL and SeqN.txt - Relative time is mismatched between the two editors - https://github.com/NASA-AMMOS/aerie-ui/pull/1324
- [x] Add unit test to verify time calculations are correct - https://github.com/NASA-AMMOS/aerie-ui/pull/1324
- [ ] Add Sequence Adaptation name - https://github.com/NASA-AMMOS/aerie-ui/pull/1329
- [ ] Disable save button in parcel form if user has not provided a parcel name
- [ ] Prevent dictionary file upload re-submission during upload
- [ ] Fix overflow on sequencing page when seq JSON view is collapsed (default). Causes nasty editor tooltip overflow resizing issues
- [ ] The derived `$parcel` can still be theoretically `undefined`. I suggest we use `find` instead of `filter` and explicitly return `null` if it's not found
### Optional Tasks
- [ ] `fswCommandArgDefault` in command-dictionary.ts can be in a switch statement - `IN PROGRESS`
- [ ] can we change queries like `DELETE_PARCEL_TO_PARAMETER_DICTIONARIES` to be more English friendly like `DELETE_PARCEL_DICTIONARY_ASSOCIATION`? - `IN PROGRESS`
- [ ] More comments in functions with long and deeply nested conditional statements like `commandLinter`
joswig commented 1 month ago

regarding the nullish coalescing on ArgumentTooltip, I'm good showing 'None' here, we can't safely infer a default if the value is unspecified in the dictionary. We could hide the row completely but that seems less obvious.