-
Notifications
You must be signed in to change notification settings - Fork 113
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
[KED-2988] Fix for plotly when escape button pressed #654
Conversation
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.
Great work! Just two small things and then we're good :)
@@ -21,7 +21,7 @@ | |||
right: 0; | |||
bottom: 0; | |||
left: 0; | |||
z-index: 3; | |||
z-index: 5; |
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.
Given we now have a global toolbar, that toolbar should almost always be visible.
With that in mind, I think we should keep the z-index
at 3 and then bump over the the left
value to 80px
, since that's the width of the global toolbar.
There's actually a SCSS variable you can use for this here.
Co-authored-by: Tynan DeBold <[email protected]>
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.
Works like a charm :)
I've left one small comment and I'll approve now. Great work.
@@ -20,7 +20,7 @@ | |||
top: 0; | |||
right: 0; | |||
bottom: 0; | |||
left: 0; | |||
left: 80px; |
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.
I would suggest changing this hard-coded value to the variable we have set in the variables
file, like we do here.
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 just a minor comment
src/components/plotly-modal/index.js
Outdated
{metadata.node.name} | ||
</span> | ||
<> | ||
{metadata && metadata.plot && ( |
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.
you can potentially move this check to line 18 and short circuit there. IMO it's cleaner that way.
if (!visible.plotModal || metadata?.plot) {
return null
}
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.
Optional chaining for the win. Forgot this also works in vanilla JS, not just TS.
@rashidakanchwala I agree with Lim here. This is cleaner.
Description
Jira Ticket - https://jira.quantumblack.com/browse/KED-2988
Development notes
Besides fixing the bug, I have also raised z-index for plotly modal so it comes on top of the global toolbar.Update - Plotly Modal when expanded will show the global toolbar on the left so users can access settings/themes while viewing the modal (use-case - if they want to switch between light/dark theme while viewing the charts)
QA notes
Steps to produce the bug --
Run shuttle factory with Kedro-viz --> Click on node Uplift Report Figures' -- > Expland plotly chart --> Press escape
Checklist
RELEASE.md
file