Skip to content
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

Merged
merged 1 commit into from Aug 19, 2020
Merged

Implement Photon design for the viewer #11077

merged 1 commit into from Aug 19, 2020

Conversation

sawanm9000
Copy link

@sawanm9000 sawanm9000 commented Aug 18, 2019

  • Flat icons
  • Larger buttons make touch input easier
  • No textures, unnecessary shadows and gradients - The overall UI is less taxing on the brain.
  • No useless animations - Actions like hovering is registered fast - The overall UI now feels snappy.
  • Lighter (as in kilobytes to load)
  • New UI match with Firefox's UI.

Edit: Latest screenshot:
PDF js Dark

PDF js Light

Old look for comparison:
OLD PDF js

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Aug 18, 2019

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 package.json related changes belong in a patch like this.

Edit: Just from looking at the GIF, two things stand out:

  • The next/previous icons, in the Findbar, look horizontally off-centre on the buttons.
  • On the right-hand side of the Zoom dropdown, the border looks "weird".

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a 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.
pageNumber_focus

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!?

web/viewer.css Outdated Show resolved Hide resolved
web/viewer.css Outdated Show resolved Hide resolved
@sawanm9000
Copy link
Author

sawanm9000 commented Aug 20, 2019

The findbar now looks a lot like that of Firefox:
Firefox findbar

@timvandermeij
Copy link
Contributor

/botio-linux preview

@timvandermeij
Copy link
Contributor

I'm noticing the following things:

  • When the viewer is loading, the page number indicator has a red border.
  • Almost all icons appear to be off-center on hover (hover over the + in the toolbar for example and notice that the space on the right/bottom is larger than on the left/top).
  • If I compare the design with the existing one, the background color of the toolbar stands out for me. The one from the existing design has a color that is almost equal to the background color of the viewer itself, whereas in this design the toolbar appears to be a lot lighter, which looks a bit strange to me.

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?

@timvandermeij
Copy link
Contributor

/botio-linux preview

@designakt
Copy link

@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.
We only noticed a few things:

  • a few minor tweaks

    • in Firefox we do not need the “open with” icon (currently don't have that either)
    • when clicking download, default should be safe file (as is now)
    • the hover area seems to be bigger then on other Firefox buttons - would be great to align
    • on mac the dropdown seems to have a glitch on top and bottom
      CleanShot 2020-07-15 at 09 23 26@2x
  • a really nice to have
    Ideally we would also have the toolbar align with Firefox toolbar style - dark in dark mode, and light in light mode. - With a line separating the pdf toolbar from what is above.

  • and a question
    We like the new presentation mode icon, but were wondering what got us to use this over the previous symbol. Looking at it we noticed that we seem to mix "presentation mode" and "fullscreen" descriptions in this feature. Do we know that presentation mode is better understood than fullscreen?

For any follow-up questions please ping @aaronrbenson

@brendandahl
Copy link
Contributor

@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?

@sawanm9000
Copy link
Author

sawanm9000 commented Jul 16, 2020

when clicking download, default should be safe file (as is now)

So should I create a "floppy disc" icon or use the existing 'download' icon in firefox?

on mac the dropdown seems to have a glitch on top and bottom

I don't have a Mac so I'm going to need help on this.

Do we know that presentation mode is better understood than fullscreen?

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:
Pinging @aaronrbenson
This is how the dropdown button and a field look like respectively:
DropDownFieldButton

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
I'll work on it during the weekends.

@nt1m
Copy link
Contributor

nt1m commented Jul 16, 2020

If this is blocked on the macOS select issue, I would recommend ignoring it for now, since this affects all <select> in general. I'm happy to look into fixing the root issue in Firefox, though I'm first looking if there's an existing bug filed.

@nt1m
Copy link
Contributor

nt1m commented Jul 16, 2020

I'm looking into it in: https://bugzilla.mozilla.org/show_bug.cgi?id=1653382

@lundjordan lundjordan added this to Started in Form fill Jul 27, 2020
@sawanm9000
Copy link
Author

New update: Photon Design with dark and light theme and SVG icons.

Compare

PDF js Dark

PDF js Light

High DPI preview:

PDF js Dark HiDPI

PDF js Light HiDPI

@sawanm9000
Copy link
Author

/botio-linux preview

@bwinton
Copy link

bwinton commented Jul 30, 2020

@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)? 😃

@sawanm9000
Copy link
Author

@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.

@aaronrbenson
Copy link

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. :shipit:

@timvandermeij
Copy link
Contributor

/botio-linux preview

1 similar comment
@timvandermeij
Copy link
Contributor

/botio-linux preview

@timvandermeij timvandermeij changed the title Updated look Implement a Photon design for the viewer Aug 2, 2020
@timvandermeij timvandermeij changed the title Implement a Photon design for the viewer Implement Photon design for the viewer Aug 2, 2020
@timvandermeij
Copy link
Contributor

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!

@sawanm9000
Copy link
Author

the bookmark button has much more padding on hover than the other buttons

Fixed.

web/viewer.css Outdated Show resolved Hide resolved
web/viewer.css Outdated Show resolved Hide resolved
web/viewer.css Outdated Show resolved Hide resolved
web/viewer.css Outdated Show resolved Hide resolved
web/viewer.css Outdated Show resolved Hide resolved
web/viewer.css Outdated Show resolved Hide resolved
web/viewer.css Outdated Show resolved Hide resolved
web/viewer.css Outdated Show resolved Hide resolved
web/viewer.css Outdated Show resolved Hide resolved
web/viewer.css Outdated Show resolved Hide resolved
@fgilio
Copy link

fgilio commented Aug 2, 2020

This looks really good 😍
Thanks @utopianknight!

web/viewer.css Outdated Show resolved Hide resolved
@Snuffleupagus
Copy link
Collaborator

The following is a, possibly incomplete, list of issues/bugs that's fixed by this PR:

@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/e78745899c17fb8/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/e78745899c17fb8/output.txt

Total script time: 3.25 mins

Published

@timvandermeij timvandermeij merged commit 965d20d into mozilla:master Aug 19, 2020
@timvandermeij
Copy link
Contributor

Looks good to me now. Thank you for all your work!

@szaimen
Copy link

szaimen commented Aug 19, 2020

🎉

@sawanm9000
Copy link
Author

Awesome! Great job everyone.

@rvandermeulen
Copy link

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?

@timvandermeij
Copy link
Contributor

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.

@sawanm9000
Copy link
Author

sawanm9000 commented Aug 22, 2020

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:

Annotation 2020-08-22 110353

So we have two options:

  • Use the existing icon and have smaller file size.
  • Create a new icon and make it look just like in Firefox.

@Snuffleupagus
Copy link
Collaborator

Use the existing icon and have smaller file size.

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 findbarButton-next.svg in .dropdownToolbarButton::after { ... } would look strange.

Create a new icon and make it look just like in Firefox.

If possible, that would probably be the nicest solution as far as I'm concerned.

@sawanm9000
Copy link
Author

PR #12260

dropdown

@timvandermeij
Copy link
Contributor

@rvandermeulen The pull request that fixes the icon issue has just been merged, so the next Firefox pull should not have this problem anymore.

@natewind

This comment was marked as abuse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Form fill
  
Done
Development

Successfully merging this pull request may close these issues.

None yet