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

slideshow buttons #1149

Merged
merged 19 commits into from Sep 15, 2019
Merged

slideshow buttons #1149

merged 19 commits into from Sep 15, 2019

Conversation

IzabelaBakollari
Copy link
Contributor

@IzabelaBakollari IzabelaBakollari commented Mar 18, 2019

Added slideshow buttons and changed color of the link "Host your own server".

sharing

@camilasan camilasan added this to the 2.5.3 🐛 Bug fixes milestone Mar 21, 2019
@camilasan camilasan added this to In Review | Testing in Desktop Client 2.5 Series via automation Mar 21, 2019
@camilasan camilasan added enhancement enhancement of a already implemented feature/code design 2. to review labels Mar 21, 2019
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great design-wise! :)

@DominiqueFuchs
Copy link
Contributor

DominiqueFuchs commented Sep 11, 2019

After digging this PR out I ran some tests to see if this could happily be merged after the positive design reactions.

Unfortunately, several months later the result is a mess without further modifications (screen attached):

  • Redundant hosting link (thus resulting in broken layout)
  • The arrows only look good on the dark background, Windows-white is obviously no good contrast

Any comments if fixing and adaption to current state is still desired? If yes, I could do this (should @IzabelaBakollari be out of reach in the meantime), otherwise I'd close this.

SlideshowArrows

@jancborchardt
Copy link
Member

@DominiqueFuchs would really be cool to fix this if you are up for it. :) It was reported as an accessibility issue.

…in wizard

Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>
Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>
…itions through the last months)

Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>
Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>
Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>
@DominiqueFuchs
Copy link
Contributor

Done:

  • Recreated the prev/next-buttons, as the original svg contained reduced blur filters as shadows that are not supported by the Qt SVG renderer, resulted in strange looks when viewed at high dpi or scaled up. Thus I made a new vector image with a slightly thicker arrow instead, rounded the corners a bit to fit into the much rounding in the current CD and applied a slight gradient to maintain 'intuitive' highlighting. Also, I used the NC blue as a base, as this gives good contrast on every background that makes sense here
  • Implemented in wizard page layout from master
  • Speaking of accessibility, I embedded the 'Host your own server' link in his own widget - otherwise it is not tab-accessible (QLabels itself aren't)

Builds fine everywhere, looks fine on Windows (only build/test system with a GUI on my side ATM)

Button svg:

NextBackButtons

NextButtonBG

Build result:

nc-slideshow-buttons

🦊 approve, @jancborchardt ?

@jancborchardt
Copy link
Member

@DominiqueFuchs nice work! :)

The only thing is that there’s too much blue there. Only the header and the primary button (in this case "Register with a provider") should be blue now, but not the prev/next icons, and also not the icons and the text. So likely one version for light backgrounds and one for dark.

But we could also do that in a separate pull request to do small steps but quickly – also cause text and icons being blue was like that before already. :)

…for wizard slide buttons

Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>
Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>
…ility refinements and thus implementation of helper fct. to retrieve themed QIcons.

Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>
…ection logic

Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>
Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>
@DominiqueFuchs
Copy link
Contributor

@jancborchardt Sorry, before your last comment I did not notice the referenced issue, thanks for clarifying anyway!

Summary of changes:

  • Desired design changes
  • Implementation of a helper function in theme.h/theme.cpp to retrieve a QIcon resource based on current OS theming state (meaning here bg color of the widget, which is actually only OS controlled as our current 'app theming' only applies to systray icons).

Windows (Gnome also tested, runs fine but looks essentially the same as expected):
ThemedWizSlideCntrl_Win
KDE:
ThemedWizSlideCntrl_KDE

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff! 👍 I have some more design ideas for the setup process, but that’s a follow-up pull request, maybe best at the conf or at a contributor week. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. to review accessibility design enhancement enhancement of a already implemented feature/code
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants