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

Added changes to UI of the admin_add_login page #3873

Merged
merged 1 commit into from Nov 16, 2018

Conversation

Projects
None yet
6 participants
@nightwarrior-xxx
Copy link
Contributor

nightwarrior-xxx commented Oct 12, 2018

Status

Ready for review

Description of Changes

Fixes #3746

Changes proposed in this pull request:

Added changes to the admin_add_login page

@nightwarrior-xxx

This comment has been minimized.

Copy link
Contributor

nightwarrior-xxx commented Oct 12, 2018

@SaptakS Please review.I made a new PR as I was unable to pull changes from upstream.I will not repeat this mistake next time.

@nightwarrior-xxx nightwarrior-xxx changed the title Added changes to UI of the admin_add_user page Added changes to UI of the admin_add_login page Oct 12, 2018

@eloquence

This comment has been minimized.

Copy link
Contributor

eloquence commented Oct 15, 2018

I would recommend re-scoping this PR to only fix the actual problem identified in #3746 and handle other design changes separately.

Regarding the actual text, we should ensure we have consensus on what the recommendations are.

"Username must not contain your original name"

Do you mean to say "real name"? If so, what is the underlying security reason?

"Username must be unique"

I don't know how useful this one is; as far as @huertanix stated in #3746, the main questions that come up are case-sensitivity and spaces. We should not add text just for the sake of adding text; most systems require usernames to be unique, so if this is not a common point of confusion, I don't think we need to say so here.

We should, however, state if the username may contain spaces (it can).

@nightwarrior-xxx

This comment has been minimized.

Copy link
Contributor

nightwarrior-xxx commented Oct 16, 2018

@eloquence Gotcha.That means we should just add only these points

  • Username can contain spaces.
  • Username is case sensitive.
@eloquence

This comment has been minimized.

Copy link
Contributor

eloquence commented Oct 16, 2018

Yes, and per my earlier comment, I suggest we remove the other style changes from this PR. Keeping PRs scoped clearly to the issues they resolve simplifies review.

Once this is done, could you also upload a before/after screenshot? @ninavizz may have some final styling suggestions.

Thanks a lot! :)

@nightwarrior-xxx

This comment has been minimized.

Copy link
Contributor

nightwarrior-xxx commented Oct 18, 2018

@eloquence @ninavizz Please review .
Page before modification
currentsecuredropaddadminuserpage

Page after modification
page after modification

Regards,
@nightwarrior-xxx

@eloquence

This comment has been minimized.

Copy link
Contributor

eloquence commented Oct 20, 2018

Thanks, @nightwarrior-xxx ! I would suggest two changes:

  • Don't add a "Username" label to the username input. To be sure, there are arguments in favor of labels; the problem right now is one of inconsistency. The other input on this form lacks a label, as does the sign-in form. If we want to have a discussion about adding labels to inputs, it should be done consistently across the board, and separately from this PR.

  • Remove the new styling of the password. This is a separate issue from the one you're looking to resolve with this PR, and additional style changes should be applied consistently and discussed separately.

@nightwarrior-xxx

This comment has been minimized.

Copy link
Contributor

nightwarrior-xxx commented Oct 25, 2018

Hey,
@eloquence and @ninavizz Please review!!!

current page
currentsecuredropaddadminuserpage
page after modification
page_after_modifications2

Regards,
@nightwarrior-xxx

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 25, 2018

Codecov Report

Merging #3873 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #3873   +/-   ##
========================================
  Coverage    84.54%   84.54%           
========================================
  Files           43       43           
  Lines         2724     2724           
  Branches       296      296           
========================================
  Hits          2303     2303           
  Misses         354      354           
  Partials        67       67

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update daa9222...3b009bc. Read the comment docs.

@ninavizz

This comment has been minimized.

Copy link

ninavizz commented Oct 25, 2018

@nightwarrior-xxx Looks good to me for asap deploy; getting fussy on things, I'd love an &nbsp; before the "A" in "Add" on the button (or maybe 5px of right side padding after the "+" icon), and maybe 5-10px of padding between the HOTP field and the checkbox text above it. The latter points are not blockers, tho. Thx!! :)

@eloquence

This comment has been minimized.

Copy link
Contributor

eloquence commented Oct 25, 2018

Let's keep this out of the current PR, @ninavizz, I'd prefer to keep this scoped exactly to fixing the username issue identified in the bug. Thanks :)

.journalist-username__notes
font: 12px Sans-Serif
font-weight: normal
letter-spacing: 0.05px

This comment has been minimized.

@eloquence

eloquence Oct 25, 2018

Contributor

Why is this here? I cannot visually tell much of a difference from this fraction of a pixel increase.

This comment has been minimized.

@ninavizz

ninavizz Oct 25, 2018

.05 looks like an erroneous attempt at entering .5, perhaps? I <3 .5px letterspacing, personally.

@eloquence

This comment has been minimized.

Copy link
Contributor

eloquence commented Oct 25, 2018

Aside from the minor question about the CSS, I have no concerns about this PR. There's a slight increase in whitespace above the username field, but I don't see much of a consistency issue with the rest of the form.

// This query is to reorder the flex box elements so that they correctly form the
// two boxes (submit/returning) when the screen is sized down
@media only screen and (max-width: 768px)
.container
display: block
display: block

This comment has been minimized.

@eloquence

eloquence Oct 25, 2018

Contributor

Tiny nit: .editorconfig in the root specifies a newline at the end of file; this shouldn't be modified in this PR.

This comment has been minimized.

@heartsucker

heartsucker Nov 10, 2018

Contributor

Hmm. My comment in the review doesn't seem to be appearing. Anyway. This is still missing a newline at the EOF.

@nightwarrior-xxx

This comment has been minimized.

Copy link
Contributor

nightwarrior-xxx commented Oct 30, 2018

@ninavizz @eloquence
Please review!!
Before changes
currentsecuredropaddadminuserpage
After Modifications
page_after_modification3

@nightwarrior-xxx nightwarrior-xxx force-pushed the nightwarrior-xxx:change branch 3 times, most recently from 1103544 to 3877426 Nov 7, 2018

@nightwarrior-xxx

This comment has been minimized.

Copy link
Contributor

nightwarrior-xxx commented Nov 10, 2018

@heartsucker Please review

@heartsucker
Copy link
Contributor

heartsucker left a comment

Looks good. Sorry this is taking a long time to get merged. Some PRs that have a UX component take number of iterations to get right. There's one comment from me on the technical side.

@nightwarrior-xxx

This comment has been minimized.

Copy link
Contributor

nightwarrior-xxx commented Nov 11, 2018

@heartsucker and @redshiftzero Please review

@nightwarrior-xxx nightwarrior-xxx force-pushed the nightwarrior-xxx:change branch 2 times, most recently from dae8235 to 653b97d Nov 13, 2018

@nightwarrior-xxx nightwarrior-xxx force-pushed the nightwarrior-xxx:change branch from 653b97d to 057a16a Nov 15, 2018

@redshiftzero
Copy link
Member

redshiftzero left a comment

thanks for your patience, 👍 to merge

@redshiftzero redshiftzero merged commit dc5a96b into freedomofpress:develop Nov 16, 2018

5 checks passed

ci/circleci: admin-tests Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: staging-test-with-rebase Your tests passed on CircleCI!
Details
ci/circleci: tests Your tests passed on CircleCI!
Details
ci/circleci: updater-gui-tests Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment