-
Notifications
You must be signed in to change notification settings - Fork 10
Enterprise: Handle browser crash #291
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
Conversation
6f4ba5c to
343de12
Compare
b78cbeb to
b9def40
Compare
|
I'll fix the lint issues here, and keep this UX for now. We will improve later. |
As discussed last week, please get the UX approved by Matt and add the correct designs first. |
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.
As re-discussed, if this is urgent to land, because QA needs the visible feedback about the crashes, then I'm ok with landing this now and follow-up on the UX once that is defined.
Additionally I have some suggestions about the code structure.
b9def40 to
5854a51
Compare
1rneh
left a comment
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! Just some smaller things... Only important thing left is to clarify the intention of the time period.
5854a51 to
6f896cd
Compare
1rneh
left a comment
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 now, thanks!
6f896cd to
69e6d2e
Compare
| /* stylelint-disable-next-line */ | ||
| left: 8px; | ||
| font-size: medium; | ||
| /* stylelint-disable-next-line stylelint-plugin-mozilla/use-design-tokens */ |
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.
Was this lint exception actually needed? Didn't you forget to remove this?
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.
Yes, https://treeherder.mozilla.org/logviewer?job_id=543562202&repo=enterprise-firefox-pr&task=DGk37lvMQKqsKjfUW9OEYA.0&lineNumber=212. We agreed that once we have a well defined crash ux, this will be revamped anyhow.
UX is open to discussion.