-
Notifications
You must be signed in to change notification settings - Fork 8
Feat migrate core to new oscd UI components #28
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
Feat migrate core to new oscd UI components #28
Conversation
1cd6ee4
to
27bb7cc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this work, the substantial changes you made are looking great and working fine on my machine! Also, the improvements to naming and CI that you made are much appreciated, thanks for putting in the extra effort on those fronts.
There are three very minor things I've noticed about the incidental changes included in this pull request:
- husky pre-commit hooks don't seem to be automatically registered any more on my machine after the bump from version
4
to version9
. Is this still working for you? If not, do we maybe need to add something to our pre-install script to register them? Also, what do you think about addingcommitlint
to our pre-commit hooks? - Some of the visual changes in the "update screenshots" commit seem unintentional to me at first glance, like a different tab being selected (maybe 1-based vs 0-based indexing?), tab selection looking different, or the app bar seeming slightly taller than before. Have you checked the visual diffs at the end, and if so, are these changes intended?
- I don't really see the need for separating the tests for editing/plugging/oscd-shell any more, since they haven't been separately testable mixins in a while. Do you think the same way, and if so, would you consider consolidating all unit tests into a single
oscd-shell.spec.ts
?
4898ec4
to
2ebcb5f
Compare
@ca-d thanks for your feedback.
Other smaller changes also thrown in:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, great work!
Also, thanks for figuring out that meaning question.
6009ce4
to
94211dc
Compare
No description provided.