aerie-ui icon indicating copy to clipboard operation
aerie-ui copied to clipboard

Sequence Editor Followup

Open duranb opened this issue 1 year ago • 0 comments

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
- [ ] https://github.com/NASA-AMMOS/aerie-ui/issues/1292
- [ ] `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`
- [ ] `ArgTitle.svelte` - title should be set to variable that is set in a reactive statement to prevent unnecessary re-renders
- [ ] Expansion/Parcel/Sequence update queries should be combined into one
- [x] `NumEditor.svelte` - input element should be of type `number` instead of `string`.
- [x] `NumEditor.svelte` - cleanup unused code
- [ ] `SelectedCommand.svelte` - address TODOs
- [x] Repeat arg hovering doesn't trigger a tooltip in editor
- [ ] Remove `utils.ts` as that was renamed to `codemirror-utils.ts`
- [ ] Add unit tests for all the new utility TS files under `/codemirror` and `/new-sequence-editor`
- [ ] `dictionaries/+page.svelte` - the `'Adaptation'` case can be changed to use the enum representation
- [ ] 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
- [ ] `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.
- [ ] `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
- [ ] Can `time-utils.ts` be combined with our existing time util file?
- [ ] `grammar.test.ts` - update to be more understandable and less fragile @joswig?
- [ ] Delete `aerie-phoenix-wordmark.svg` if not being used @joswig
- [ ] `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)
- [ ] Prevent clicking on download/copy buttons when nothing is selected @goetzrrGit
- [ ] Vertically align the "filter to my sequences" checkbox and text on the sequencing page
- [ ] Style the editor tooltip for better legibility
- [ ] Style the Selected Command section with proper header and Aerie components
- [ ] Discuss why the sequence editor has its own branding
- [ ] Empty state for third column in sequence editor
- [ ] Document usage of `globalThis`
- [ ] Rename "new-sequence-editor" folder to "sequence-editor" in utils
- [ ] De-dupe and comment `isTimeBalanced` which can be found in both `time-utils.ts` and `timeDiagnostics.ts`
- [ ] More docstrings for `time-utils.ts` functions
- [ ] Move the new dependencies from `devDependencies` to `dependencies` in package.json
### Optional Tasks
- [ ] `fswCommandArgDefault` in command-dictionary.ts can be in a switch statement
- [ ] can we change queries like `DELETE_PARCEL_TO_PARAMETER_DICTIONARIES` to be more English friendly like `DELETE_PARCEL_DICTIONARY_ASSOCIATION`?
- [ ] More comments in functions with long and deeply nested conditional statements like `commandLinter`

duranb avatar May 20 '24 23:05 duranb