Dev sequencing #1275
Merged
Dev sequencing #1275
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
duranb
requested changes
May 13, 2024
There was a problem hiding this comment.
A few superficial things:
- I'd really like to see this as not a submodule of the project. It looks like moving it under
/srcshouldn't affect anything from a cursory look. - Can we change all the kebab cased svelte files to match the current convention of CamelCased?
- Can we explicitly declare every variable and its type in the svelte files? I know svelte lets us just implicitly define variables in the reactive statements, but I feel that explicitly defining the type on declaration helps not make mistakes later on if we have to refactor.
duranb
requested changes
May 14, 2024
There was a problem hiding this comment.
Thanks for finding a way to merge the sub module @cohansen! I'm still working through this, but I've posted my initial thoughts so far.
Just a heads up so you can start thinking about it, one thing that I would like is to have unit tests for all the new utility TS files. It doesn't have to be for this PR if you don't think you have time, but I will be asking for it in the next sprint.
Also, do you mind fleshing out the PR description a little more? Ideally, it would be nice to have a testing procedure of sorts.
…enamed new svelte files to match our standard
* Sequence Editor changes: * Modify button placement ( they are pretty ugly) * SeqJSON editor is collapsable now * Prevent the user from shrinking the panel too small * vertically align icons in button * remove store for sequence editor rows * change mime type of text file * style css gutter to allow for non-interactive display * cleanup comments * remove typo * remove typo --------- Co-authored-by: bduran <bryan.duran@jpl.nasa.gov>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Depends on: NASA-AMMOS/aerie#1444
Easiest rebase of my life? I was expecting this to be a lot worse. Needs thorough testing and review.
Adds the new sequence editor and a major refactor of how we are using dictionaries. All dictionaries are now part of a parcel that needs to be assigned to items that previously pointed directly to a command dictionary.
To Test:
Everything else should be working nominally.