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

Create and place auth widget #278

Merged
merged 1 commit into from
Mar 26, 2019
Merged

Create and place auth widget #278

merged 1 commit into from
Mar 26, 2019

Conversation

sssoleileraaa
Copy link
Contributor

@sssoleileraaa sssoleileraaa commented Mar 20, 2019

Description

First iteration of #264

demo2

What changed?

13 securedrop_client/gui/main.py

  • Moved refresh status bar out of main.py into its own StatusBar class inside widgets.py

187 securedrop_client/gui/widgets.py

  • Added StatusBar class and moved refresh logic into here. Currently the refresh button is on the right side of the status bar. Will need to write our on QStatusBar class in order to have a permanent left-side widget in a status bar. QStatusBar only provldes a way to have a right-side status bar widget that doesn't get replaced by messages. This is work that can be done as part of the Refresh Widget Styles documented here: UX parity between prototypes & coded client #261
  • Refresh button has moved from next to journalist name to the top status bar
  • Implemented auth logic documented here: [client styling] Create and place auth widget #264 - sign-in button now takes up the entire width of the left pane when user is logged out and the sign-out option is an option in a menu next to the journalist username
  • Switched out the dropdown arrow next to the journalist username with the svg from the inventory list
  • Added JournalistMenu class to provide a sign-out option
  • Added JournalistMenuButton class to display the journalist menu when clicked

11 securedrop_client/resources/images/dropdown_arrow.svg

  • Added dropdown arrow svg from inventory list

359 securedrop_client/resources/images/hexes.svg

  • Added an svg of hexes, because I wanted to learn how to make a gradient fill, but also as a placeholder

BIN +17.7 KB securedrop_client/resources/images/logo.png

  • Made the logo smaller to fit in the 200px side bar

What's left?

  • Verify/ update hex, rgb, or rgba colors for left pane - I grabbed colors from the screenshots in [client styling] Create and place auth widget #264 using Gimp, but the light blue journalist name and sign-in button didn't look too great against the light gray background, and I'm not sure if we plan to have a darker background because I believe the branding image is still in progress. I tried searching for rgb values in a ticket somewhere, but I think we haven't decided on the exact color scheme for menus, buttons, etc. yet, or I'm just bad at searching through the ux repo (super likely). This looks promising: Complete branded bits & color schema securedrop-ux#36 but still can't find rgb or hex values
  • Make clicking on the username also open the sign-out drop-down menu
  • Make selected state of sign-in button more obvious (right now tabbing to select the button just gives it a slightly darker border)

@sssoleileraaa sssoleileraaa force-pushed the issue-264-auth-widget branch from 9d6e091 to 253ddd9 Compare March 20, 2019 23:26
@sssoleileraaa sssoleileraaa marked this pull request as ready for review March 21, 2019 15:45
@sssoleileraaa sssoleileraaa force-pushed the issue-264-auth-widget branch 3 times, most recently from d69a0c4 to 5f845dc Compare March 21, 2019 23:16
Copy link
Contributor

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

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

this looks amazing! 🚀 just a couple of questions/comments inline

self.user_state.setText(_('Signed out.'))
self.login.setVisible(True)
self.logout.setVisible(False)
self.refresh.setVisible(False)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think when we log out we want the refresh button to be hidden no?

Copy link
Member

Choose a reason for hiding this comment

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

@creviera asked me about where the Offline icon was, yesterday—did you want to put that in on a separate PR or were you waiting for me to respond to your Slack, Allie? All icons are in table on this Issue: freedomofpress/securedrop-ux#17

Copy link
Contributor Author

@sssoleileraaa sssoleileraaa Mar 22, 2019

Choose a reason for hiding this comment

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

Oh yeah that's right! The new offline icon will be in the pr for this issue: #279. As I understand it, there will be three icons which are in this inventory list: freedomofpress/securedrop-ux#17. There you'll see an icon labeled "offline", the static refresh icon, and the active refresh icon.

@redshiftzero - Realizing now that when I moved the refresh icon to the StatusBar class, I didn't move this check, so yeah I didn't catch that I changed the behavior. Good catch. However, I'm working on a follow-up PR to change and test the new behavior of refresh, so if you're cool with leaving this as is for now, I'm cool with it. 😎

Copy link
Contributor

Choose a reason for hiding this comment

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

We discussed this and decided that since the refresh icon appearing when not logged in is a regression it would be good to resolve as part of this PR

''')
arrow = load_image("dropdown_arrow.svg")
self.setIcon(QIcon(arrow))
self.setMinimumSize(20, 20)
Copy link
Contributor

Choose a reason for hiding this comment

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

is it me or do these pixmaps look kind of blocky? (just for the dropdown SVG and the refresh SVG)

Screen Shot 2019-03-21 at 4 42 30 PM

note that it's possible this is a macOS only thing - let me know if so and we can disregard since we're targeting Linux.

Copy link
Member

Choose a reason for hiding this comment

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

Blarg—is that crappy art on my part, @creviera? If so, happy to re-rip another one... 🤢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ninavizz - Some higher fidelity images would be great! I'm currently using the svgs provided in the inventory list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ninavizz and @redshiftzero - In a follow-up PR I was able to improve the look of the refresh icon by changing this (which looked pixely/blocky):

        refresh_pixmap = load_image('refresh.svg')
        self.refresh.setIcon(QIcon(refresh_pixmap))

To the following code:

        pixmap = load_image('refresh.svg')
        scaled = pixmap.scaled(18, 18, Qt.KeepAspectRatio, transformMode=Qt.SmoothTransformation)
        icon.addPixmap(scaled, QIcon.Normal)

But I think I need to do more research into how to work with SVGs and QIcons. I may need to create a QSvgWidget which are used to display the contents of SVG files: https://doc.qt.io/archives/qt-5.10/qtsvg-module.html

tb.user_state.setText.assert_called_once_with('Signed out.')
tb.login.setVisible.assert_called_once_with(True)
tb.logout.setVisible.assert_called_once_with(False)
tb.refresh.setVisible.assert_called_once_with(False)
Copy link
Contributor

Choose a reason for hiding this comment

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

(and we should keep this assert in the test once we hide the refresh button after logout)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same response as my other comment about the upcoming PR. But now that I'm thinking about this more, I can switch the behavior back so this is a completely tested PR, and then just update the tests later.

@eloquence eloquence changed the title Issue 264 auth widget Create and place auth widget Mar 22, 2019
@sssoleileraaa sssoleileraaa force-pushed the issue-264-auth-widget branch from 5f845dc to 229a0d3 Compare March 26, 2019 19:56
@sssoleileraaa
Copy link
Contributor Author

@redshiftzero - refresh behavior is the same as it used to be with new tests

Current and old behavior:

  • When login dialog opens, refresh button shows in the background window
  • If you click login the refresh button works
  • If you close login window without logging in, refresh button grays out when you try to use it
  • If you click logout, refresh button is hidden

@sssoleileraaa sssoleileraaa force-pushed the issue-264-auth-widget branch from 229a0d3 to 2bd3f33 Compare March 26, 2019 20:06
@sssoleileraaa sssoleileraaa force-pushed the issue-264-auth-widget branch from 4d53856 to e2ba31a Compare March 26, 2019 20:24
Copy link
Contributor

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

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

looks good now, thanks!

@redshiftzero redshiftzero merged commit 914b145 into master Mar 26, 2019
@redshiftzero redshiftzero deleted the issue-264-auth-widget branch March 26, 2019 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants