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

Small refactors #174

Merged
merged 4 commits into from
Aug 15, 2024
Merged

Small refactors #174

merged 4 commits into from
Aug 15, 2024

Conversation

jafeltra
Copy link
Collaborator

@jafeltra jafeltra commented Aug 9, 2024

Description: The PR makes a few small changes to the structure of the code to follow best practices:

  • Renamed components to not include the word "Component" in the file name or component name
    • CodeMirrorComponent --> CodeEditor
    • ConsoleComponent --> Console
  • Renamed the editor components to be "Editors" rather than "Output"
    • they do more than just display output, and this lines up with the new CodeEditor component (for example, now a FSHEditor renders a CodeEditor with a specific language mode)
    • FSHOutput --> FSHEditor
    • JSONOutput --> JSONEditor
  • Moved the tests out of the src folder (this better mirrors the structure in other FSH tooling repos as well)
  • Moved the application theme to its own file
  • Moved a button within the code to match where it is rendered on the screen (this just bugged me when I went to look for it and didn't find it where I expected it)

Testing Instructions: Make sure the editors and console still work, the app styling looks the same, and the tests still run.

Related Issue: No issue, but I made these changes while preparing the Knowledge Sharing Session.

- Separate out the theme into its own file
- Fix typo
- Move a button to a more logical place in the code (the css places
  it correctly regardless so the code placement is just helpful to
  those reading the code)
Copy link
Collaborator

@mint-thompson mint-thompson left a comment

Choose a reason for hiding this comment

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

I agree with the renames and reorganization. Everything looks the same when run locally, and the tests all pass locally. Thank you!

@KaelynJefferson
Copy link
Collaborator

Hi Julia, good work on the changes! I find that I have some errors when running the tests locally (one example I see is in tests/utils/FSHHelpers.test.js).

@jafeltra
Copy link
Collaborator Author

Thanks for reviewing @KaelynJefferson! We talked about this outside of this PR, so just to follow up here -- I believe all those errors your seeing being logged are actually not new to this branch. The errors are being logged from SUSHI, so there is FSH in the tests that cause errors within SUSHI. There's two things not ideal about that -- 1. it would be nice to not even have the SUSHI messages get logged. Ideally they'd be suppressed because we aren't testing anything about them in FSH Online because that isn't a FSH Online responsibility. 2. It would be nice if we didn't use FSH that created errors (unless of course one of those tests is testing that FSH Online handles FSH that results in errors).

After looking through the error messages, I think we determined that all the logged messages and errors are the same as they are on master. And since none of the tests themselves have errors (only the FSH within the tests has SUSHI errors), we should be set on that.

Let me know if I summarized anything incorrectly or if you notice anything else!

@jafeltra jafeltra merged commit 1544a2d into master Aug 15, 2024
4 checks passed
@jafeltra jafeltra deleted the updates-from-kss branch August 15, 2024 14:00
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