-
Notifications
You must be signed in to change notification settings - Fork 912
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
Add SVG logo spinner #387
Add SVG logo spinner #387
Conversation
Signed-off-by: kgcreative <kvngar@amazon.com>
✅ DCO Check Passed 6f5a26f |
… landing page Signed-off-by: kgcreative <kvngar@amazon.com>
✅ DCO Check Passed 0c29305 |
Hey Kevin, did you run tests as described in the README? |
Signed-off-by: kgcreative <kvngar@amazon.com>
All tests pass. Updated two snapshots. |
✅ DCO Check Passed 927568d |
Great! Can you also add some screenshots just for reference? |
Done and done! updated original PR with before/after screenshots. |
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 !! Thanks for making changes Kevin.
@kgcreative looks great! Thanks! Ran the functional tests and they all pass just so you know! For the card do we think we can update it to look better in dark mode or did we just want to leave it since it already was hard to read in the previous view, it looks like this: Previous had the same dark text but maybe a little bit easier to read since a vibrant green color against with black text (but still looked compared to white text). In terms on the actual SVG, do you know if we have a polyfill for Pretty weird but I pulled this down and ran the unit tests (even with the snapshot) and the unit tests keep failing with a date mismatch. So I ran on them on a pipeline and they passed, then I ran the main repo's main branch multiple times but that passes for me. It just keeps failing on the unit tests when I try your branch locally, even if I clear my node modules, which is weird. Could someone else give this a run as well locally? If another person's unit tests pass then I'm okay with merging. Thanks! |
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.
Even though I left a comment if I want to mark this review with "With Comments" github requires me to enter a comment with the popup while responding to the review 😤.
Thanks @kavilla - Re: Browser support, as of 7.9, kibana (and EUI) no longer supports IE11. See this matrix: https://www.elastic.co/support/matrix#matrix_browsers - With that in mind, do we need to revisit or create a browser support list for OpenSearch Dshboards 1.0? Re: Dark mode, let me dig into the CSS a bit and see what I can do - perhaps that might need to be a follow-up issue. How did you trigger dark mode? Switch via the OS? (I just want to make sure I can reproduce and test visually locally) |
The weird thing is the date mismatch was happening on a component I didn't even touch. I'm wondering if something else is going on with that |
Thanks! Yeah I think its worth it I think. Created an issue in the docs repo: opensearch-project/documentation-website#38
Just go to Stack Management --> Advanced Settings --> Search for Dark Mode
Might be local setup thing. If someone else pulls this down and it passes with your commit then I'm fine with just merging and looking deeper post merge. |
I pulled this PR down, ran |
@kgcreative since this is the case, did we want to revert the updating snapshot commit. Also curious how your local setup. |
Yeah, i can revert the snapshot update commit and see if that helps. For my local setup, I followed the developer guide verbatim. Is there anything I can provide you that might help troubleshoot if it's a local build issue? |
This reverts commit 927568d. Signed-off-by: kgcreative <kvngar@amazon.com>
✅ DCO Check Passed 74ae8fa |
@kavilla - After reverting the snapshot update, i'm getting a date mismatch on my jest test suite (There seems to be a 1 day date mismatch on two VegaVisualization snapshot tests, and I'm seeing the following message along with it: |
…ontrast Signed-off-by: kgcreative <kvngar@amazon.com>
@kavilla - I updated the CSS color reference in the card to use the primary color token so the Dark Mode contrast is a bit better: |
✅ DCO Check Passed 1497154 |
Yeah I pull everything down so I can run it to verify, and we can potentially create an issue to figure out if we can recreate on other machines. |
Looks really good thank you so much! |
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.
linter, unit tests, integ test, and functional tests all good. looks really slick too! thank you
* Add SVG logo spinner, remove horizontal loader * Change color on Primary "OpenSearch Dashboards" card in the main home landing page * replace hardcoded CSS value with token so dark mode card has enough contrast Signed-off-by: kgcreative <kvngar@amazon.com>
* Add SVG logo spinner, remove horizontal loader * Change color on Primary "OpenSearch Dashboards" card in the main home landing page * replace hardcoded CSS value with token so dark mode card has enough contrast Signed-off-by: kgcreative <kvngar@amazon.com>
Signed-off-by: kgcreative kvngar@amazon.com
Description
Add SVG logo spinner, remove horizontal loader.
Before
After
Update primary dashboards card to use primary brand color.
Before
After
Issues Resolved
Check List