-
Notifications
You must be signed in to change notification settings - Fork 0
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 fixture pausing with UI controls #89
Conversation
4a7eb2f
to
7bd0fbb
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.
Looking nice!
|
||
#d2l-test-controls button:focus { | ||
background-color: #cdd5dc; | ||
} |
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.
Looks duplicated from lines 54-56?
display: none !important; | ||
} | ||
|
||
#d2l-test-controls #start, #run { |
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.
Since these end up in the global namespace, I might suggest prefixing everything with d2l-test-*
(i.e. #d2l-test-run
, #d2l-test-start
) just to avoid the crazy situation where someone uses an id="start"
on one of their top-level components.
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 guess you mean to prevent them from accidentally selecting the controls? Not entirely opposed to that, but I think I'd rather convert the whole thing to a component after we get it merged.
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.
Yeah either a getElementById
type collision or in this case this CSS would target their stuff and do weird things. But if the plan is to wrap it in a shadow root, then those concerns go away.
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.
Well this targets stuff inside #d2l-test-controls
(except I missed #run
here), but point taken.
} | ||
|
||
#d2l-test-controls button.primary:focus { | ||
background-color: #004489; |
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.
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.
Ah I see this was resolved in a follow-up 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.
Yeah things are slightly messy trying to piece it all back together from the original big PR.
Co-authored-by: Dave Lockhart <[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.
Looks good -- just the duplicated CSS note, but maybe that'll get cleaned up later.
🎉 This PR is included in version 0.19.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
US152298: Visual-diff: ability to debug in the browser
This should be the biggest PR to add the main feature. Everything after should be smaller additions and tweaks.