-
Notifications
You must be signed in to change notification settings - Fork 489
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
Update button #4606
Update button #4606
Conversation
7777402
to
0320663
Compare
@@ -100,7 +100,7 @@ export class ReportFeedback extends PureComponent { | |||
|
|||
return ( | |||
<Fragment> | |||
<Fill slot="status-bar__app" group="9_feedback"> | |||
<Fill slot="status-bar__app" group="8_feedback"> |
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.
What is the reason for this change? It moves buttons around (on the status bar).
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.
To accomodate the new Update
button
Visually it looks fine to me. Two minor points on the text:
Thanks! |
return ( | ||
<React.Fragment> | ||
{updateAvailable && <Fill slot="status-bar__app" group="9_update_checks"> | ||
<button | ||
style={ { |
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.
Why are we using inline styles 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.
The footer element styles are inline (talking about Saved button and 💚 icon). I tried creating .less file to add styles initially, but it wasn't working so by looking at the reference buttons, I assumed it's not supposed to work this way. If we can eliminate the inline styles and use separate files for styles in Fill component, then we have to do it across all elements for consistency.
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 could simply add a btn--primary
class to StatusBar.less
and use it for the 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.
Updated. added a different class for Update button on StatusBar.less
8155f12
to
2733415
Compare
Ready for review |
1fdbb68
to
508a1b8
Compare
508a1b8
to
73b3af9
Compare
Mac is finally passing. Will do the final review now. |
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 did a couple of small adjustments:
- simplified CSS
- adjusted naming for clarity
- fixed whitespace
Closes #4407
Proposed Changes
Clicking
Learn what's new
will open modal with release changesClicking
Update Now
will take you to downloads page.Note: Release notes are not visible because the update behavior is being mocked.
Checklist
To ensure you provided everything we need to look at your PR:
@bpmn-io/sr
toolCloses {LINK_TO_ISSUE}
orRelated to {LINK_TO_ISSUE}