-
Notifications
You must be signed in to change notification settings - Fork 36
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
DOP-4281: Button to Open Instruqt Labs #994
Conversation
@anabellabuckvar , correct me if I am wrong, but the interactive lab should only appear if the button was clicked, if that is correct, I am not seeing that behavior in your staging with button example. Also, your staging without button example is also showing a button. |
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. I just left a comment.
Thanks for the LGTM and comments! As far as the interactive lab only appearing if the button is clicked, the interactive lab should either be present in page content if the drawer option is false, or open up after clicking the button as a drawer if the drawer option is true. There is a different PR in which Raymund created the drawer and dealt with the conditionality, so I chose to just leave the code for the Lab content itself as is in my PR. |
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.
Just wanted to leave a comment about some extra css. It's non-blocking though. Otherwise looks good
src/components/Heading.js
Outdated
display: inline-flex; | ||
height: 36px; | ||
flex-direction: column; | ||
align-items: flex-start; | ||
gap: 10px; | ||
flex-shrink: 0; | ||
margin-left: 18px; |
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.
(Maybe nit; non-blocking) A lot of these styles are already a part of LeafyGreen's Button component. I think it might make sense to double-check them, remove the ones we don't need, and only keep ones that we know we need like margin-left
to avoid additional css maintenance on our end
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.
Pretty much looks good. Just one last change
src/components/Heading.js
Outdated
role="button" | ||
className={cx(labButtonStyling)} | ||
disabled={isOpen} | ||
onClick={() => setIsOpen('True')} |
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.
We should keep this a boolean true
Stories/Links:
DOP-4281
Current Behavior:
Prod
Staging Links:
staging with button
staging without button
Notes: