-
Notifications
You must be signed in to change notification settings - Fork 381
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
Design refresh of the top bar with the menubuttons #3029
Design refresh of the top bar with the menubuttons #3029
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3029 +/- ##
==========================================
- Coverage 88.14% 88.13% -0.02%
==========================================
Files 240 240
Lines 19083 19100 +17
Branches 4871 4885 +14
==========================================
+ Hits 16821 16833 +12
- Misses 2095 2100 +5
Partials 167 167
Continue to review full report at Codecov.
|
62f803b
to
969e378
Compare
@@ -2,6 +2,10 @@ | |||
* License, v. 2.0. If a copy of the MPL was not distributed with this | |||
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | |||
|
|||
.menuButtonsPermalinkButtonButton::before { |
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.
These class names are crazy, I'll do a pass on them in a next PR. Previously we had this menuButtons
wrapper that we don't have anymore, so I think there's room to simplification.
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.
For what it's worth, I don't mind crazy class names.
border: 0; | ||
border-left: 1px solid var(--grey-30); |
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.
Replaced with some generated content where needed
e1e0dad
to
812fec1
Compare
store.dispatch( | ||
updateUrlState( | ||
stateFromLocation({ | ||
pathname: '/from-addon', | ||
search: '', | ||
hash: '', | ||
}) | ||
) | ||
); |
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.
This is made necessary because the new code throws an error when the datasource is "none" (which makes sense).
@@ -78,8 +79,32 @@ class MenuButtonsImpl extends React.PureComponent<Props> { | |||
this.props.dismissNewlyPublished(); | |||
} | |||
|
|||
_getUploadedStatus(dataSource: DataSource) { |
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'm using this util better in the next patch.
812fec1
to
2f8afcb
Compare
padding: 0 20px; | ||
padding: 0 12px; |
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.
This is a bit more compact but still good when the border are removed.
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.
Praise: Looks nice to me!
3e4a348
to
4589085
Compare
res/img/svg/link-dark.svg
Outdated
<!-- This Source Code Form is subject to the terms of the Mozilla Public | ||
- License, v. 2.0. If a copy of the MPL was not distributed with this | ||
- file, You can obtain one at http://mozilla.org/MPL/2.0/. --> | ||
<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16"><rect fill="rgba(12, 12, 13, .8)" x="7" y="3.286" width="2" height="9.429" rx="1" ry="1" transform="rotate(-45 8 8)"></rect><path fill="rgba(12, 12, 13, .8)" d="M2.354 4.522L4.485 2.39a.5.5 0 0 1 .711 0l3.19 3.19.014-.015a2 2 0 0 0 0-2.821L6.272.616a2 2 0 0 0-2.821 0L.616 3.451a2 2 0 0 0 0 2.821L2.744 8.4a1.993 1.993 0 0 0 2.8.02l-3.19-3.186a.5.5 0 0 1 0-.712zm13.062 5.237L13.287 7.63a2 2 0 0 0-2.821 0l-.015.015 3.189 3.189a.5.5 0 0 1 0 .711l-2.132 2.132a.5.5 0 0 1-.711 0L7.61 10.49a1.993 1.993 0 0 0 .02 2.8l2.128 2.128a2 2 0 0 0 2.821 0l2.835-2.835a2 2 0 0 0 .002-2.824z"></path></svg> |
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 you see, now I took the habit to name the file -dark
to make it clear that they are the "dark" version for these icons (too bad we can't use context-fill on the normal web).
4589085
to
b6bb427
Compare
Gerald's markers 2.0 migration got in front of this review, but I plan on tackling it at the beginning of next week. |
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'm marking "request changes" as I think it would be good to chat a bit real-time about some of the changes, and I think we should sort out the icon system for the header to be consistent in how the icons are presented.
I would suggest letting me use Sketch to modify the existing icons to be consistent on the 12x12 grid, as specified by the Photon guidelines. This will make the icons not blurry, have a consistent layout, and consistent stroke size. Let's chat real-time about this tomorrow.
@@ -169,15 +169,16 @@ export class ButtonWithPanel extends React.PureComponent<Props, State> { | |||
className={classNames('buttonWithPanel', className, { open })} | |||
ref={this._wrapperRef} | |||
> | |||
<input | |||
<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.
Question: What's motivating this change?
I don't mind, but it would be good to know the decision process 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.
This is so that we can add ::before
and ::after
on them. These pseudo-selectors add another inline element inside the target element. This doesn't work with an input because it has no child elements, but this works with a button.
See https://codepen.io/julienw/pen/dyXrLGg for example.
Maybe we could have done the icon system using simple background-images and left padding, but I thought that using pseudo-elements would make it possible to harness the power of flex for the positioning instead of pixel-positioning things.
* So here is the layout for this element: | ||
* <-left padding-> <-ICON (1em)-> <-4px-> CONTENT | ||
*/ | ||
width: 1em; |
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.
Suggestion: I would prefer this to be defined in pixels, as it's being used next to fonts defined in pixels. I don't want to mix and match font units, as this will make things more complicated. Right now the icons are fuzzy, and not aligned well.
The icons will resize correctly on my machine when defined in px
According to photon, icons are either defined on a 16px or 12px grid.
https://design.firefox.com/photon/visuals/iconography.html
I think we need to re-work these icons into a 12x12 grid system in order to have them consistently displayed. I think this is important to get correct now, as these are in a primary position in the UI. I'd prefer not to follow-up with this. I think it's worth chatting about it real-time tomorrow, but I can use Sketch on my computer to fix the icons to work on the 12x12 grid and be consistent.
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.
FTR I used 1em
initially, not so that it would resize when zooming, but so that if we change the font-size of an element this would adjust automatically without needing that we change the value everywhere else.
I defined them all to 12px, also also reworked the existing one for the Docs button to the 12px grid. Now the icon sizes are 12px despite the font-size being 11px, but this still looks good in my opinion so I didn't change the font-size.
TBH I don't really see the difference from the previous version :-)
@@ -132,7 +134,7 @@ class MenuButtonsImpl extends React.PureComponent<Props> { | |||
return ( | |||
<button | |||
type="button" | |||
className="menuButtonsButton menuButtonsAbortUploadButton" | |||
className="menuButtonsButton menuButtonsShareButtonButton menuButtonsButtonButton__hasIcon menuButtonsShareButtonButton__uploading" |
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.
Suggestion: In the past, we have done this as .menuButtonsShareButtonButton .has-icon .uploading
, and then in the style .menuButtonsShareButtonButton.has-icon { ... }
This is a new practice. I would prefer to keep it consistent with our existing practices. I would suggest if we want to move with a new style with this, to discuss this practice, get agreement on it, and come up with a plan to migrate our existing usages to follow this new practice.
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 had a look at our codebase, and we have several uses of this same pattern, but I think all of them are my work.
In very short, this is how BEM or CSS Guidelines suggest using modifier classes. I believe the goals are: 1. avoiding inadvertent reuse of some classes across the project, 2. reducing specificity and so avoiding specifity wars in the long run, 3. encouraging reuse for some classes by separating real utility classes from modifier classes.
I'll look at summarizing arguments for both sides on a discussion issue. In the mean time I'll look at following your suggestion and see if this makes things the same or more complicated.
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.
So I decided to keep the way I was doing here, but I changed the class names to make them shorter and less surprising.
The main reason I like using a namespace prefix is that this makes it very obvious that these classes are used only here.
I hope that will be better with the new names.
1em; | ||
} | ||
|
||
.menuButtonsMetaInfoButtonButton__uploaded::before { |
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.
See my comment on the snapshot test for the style names.
@@ -2,6 +2,10 @@ | |||
* License, v. 2.0. If a copy of the MPL was not distributed with this | |||
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | |||
|
|||
.menuButtonsPermalinkButtonButton::before { |
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.
For what it's worth, I don't mind crazy class names.
As discussed in chat, here are some 12x12 icons. Let me know if you need any changes on them. 27a96cf |
This isn't strictly necessary, but now this is more self-documented.
e0b91b9
to
9fc544f
Compare
2fc8eed
to
a876008
Compare
/* We're using ::after for borders because ::before is used for icons. */ | ||
.menuButtonsButton-hasRightBorder::after, | ||
.menuButtonsButton-hasLeftBorder::after { |
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.
Note: I'm adding better comments in a later commit.
{this._renderMetaInfoButton()} | ||
{this._renderRevertProfile()} | ||
{this._renderMetaInfoButton()} |
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.
Surprisingly, we have no test for this... I did try to write one but kept running into weird problems with jest fake timers and react-testing-library that I don't really want to debug now :/
@gregtatum I think I handled most of your comments. I added new commits that you'll identify with their "Review comments" prefix, and some others after them. Except these ones, I picked your commit with the new icons above in the stack, and I also edited my commit with my icons to remove them, but that's otherwise the only changes I made to the old commits. I'd like to highlight the main comment I didn't change: I didn't change the class names fully, but instead I made them less surprising, I hope that will be okay. I definitely intend to start a discussion issue about this topic. |
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.
Thanks for working through my review feedback. It all looks great!
Design spec
This losely follows the design spec from Figma:
![image](https://user-images.githubusercontent.com/454175/98265340-45fb2680-1f89-11eb-812e-31a89ef2b74c.png)
Of course the part with "delete" is not there yet. Also I didn't change the metainfo panel yet, I'll do that together with adding the delete button there.
I decided to go with a slight refresh, without changing the locations of the buttons.
You'll see that to be able to do that I came up with some other icons and labels than the ones defined in the spec, but I think they won't be a problem.
Screenshots
Deploy previews
production
deploy preview