-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Implement Photon design for the viewer #11077
Conversation
Referencing issue #9702 (unfortunately there's not, as far as I know, an official Photon-compatible design specification for the PDF.js viewer). Please keep in mind that none of the Edit: Just from looking at the GIF, two things stand out:
|
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.
Both of the files package.json
and package-lock.json
must be completely removed from the patch, and please make sure to follow https://github.com/mozilla/pdf.js/wiki/Squashing-Commits.
On the right-hand side of the Zoom dropdown, the border looks "weird".
This seems to be a more general problem, which actually applies to every single button on the main Toolbar.
Also, why does both the Zoom dropdown button, the Find next/previous buttons, and the Overlay buttons have a lighter background than every other button? This honestly looks quite out of place.
Edit: Furthermore, in some situations the pageNumber focus ring is now incorrectly red, see e.g.
Edit2: Also, assuming that the general design is deemed OK: I'm guessing that this is probably something which will benefit from being broken up into a couple of separate patches, to simplify testing and to reduce the risk of regressions!?
/botio-linux preview |
I'm noticing the following things:
Aside from the above, I do see improvements in this design over the existing one, but I'm definitely not a UI/UX designer. @shorlander Is this something you can chime in on? Is this something that would fit in with the current Firefox style or do you/others have different plans altogether for #9702 for example? |
/botio-linux preview |
@bwinton asked us to review the new UI before integrating into Firefox. @aaronrbenson and I agree that this is a really nice match to other Firefox UI.
For any follow-up questions please ping @aaronrbenson |
@utopianknight I know this is very delayed, but do you think you'd have time to address the above comments and we'll get this merged in? |
So should I create a "floppy disc" icon or use the existing 'download' icon in firefox?
I don't have a Mac so I'm going to need help on this.
Fullscreen button is for going full screen (it has its own icon). Presentation button is to switch to presentation mode. The reason why I changed the icon is so that it best communicate what the button actually does, which is to switch to presentation mode. Question: The reason why I changed the 'Zoom' dropdown button to make it look like a field is so that the user can type in the exact zoom level and fine-tune as they wish, while also be able to select zoom preset from a dropdown menu. This might be a solution to issue #1260. So which would be best? Should I make look like a dropdown button or leave it as it is? @brendandahl |
If this is blocked on the macOS select issue, I would recommend ignoring it for now, since this affects all |
I'm looking into it in: https://bugzilla.mozilla.org/show_bug.cgi?id=1653382 |
New update: Photon Design with dark and light theme and SVG icons. High DPI preview: |
/botio-linux preview |
@utopianknight It's pretty exciting to see the progress on this! Do you know when it will be ready for another UX review (or perhaps it's ready now)? 😃 |
@bwinton I don't know what the process is. I believe I'm finished with it but I'm also expecting your feedback so I can make further changes if necessary. |
This is looking really good, thanks for your help @utopianknight! I can put some more time and thought into this next Monday and provide more feedback (if any) but for now I think these improvements look great. |
/botio-linux preview |
1 similar comment
/botio-linux preview |
The only thing I found is that the bookmark button has much more padding on hover than the other buttons, which looks a bit strange. This is pending UX/UI approval from Mozilla, but to me it looks very good! |
Fixed. |
This looks really good 😍 |
The following is a, possibly incomplete, list of issues/bugs that's fixed by this PR:
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/e78745899c17fb8/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/e78745899c17fb8/output.txt Total script time: 3.25 mins Published |
Looks good to me now. Thank you for all your work! |
🎉 |
Awesome! Great job everyone. |
When pulling this update into Firefox, our installer packager noticed that findbarButton-next.svg and toolbarButton-menuArrow.svg are identical. I can work around that error on our end, but I wanted to ask here if that was an intentional decision? |
The images are indeed equal, but in that case I would also say that we can just re-use one image so we don't have to duplicate the SVG. I'll make a follow-up issue. |
I needed the dropdown menu icon but I couldn't find it, so I used the arrow head down here as a placeholder. Note that it's not the same as that used in Firefox. The one in Firefox is smaller. See here: So we have two options:
|
Given that the two SVG files have different semantic meaning, using the same file for both "buttons" would be a bit confusing since e.g. referencing
If possible, that would probably be the nicest solution as far as I'm concerned. |
PR #12260 |
@rvandermeulen The pull request that fixes the icon issue has just been merged, so the next Firefox pull should not have this problem anymore. |
Edit: Latest screenshot:
Old look for comparison: