clients icon indicating copy to clipboard operation
clients copied to clipboard

[PS-1512] Desktop: Fix inconsistent line height in cipher box footer

Open patrickhlauke opened this issue 1 year ago • 6 comments

Closes #3559

Type of change

- [x] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

In the box footer for a cipher in desktop - which contains "Updated", "Password updated", "Password history" - the "Password history" line is slightly taller than the other two. Line height is 17.1429px on all of them, but the last line has a button with a height of 20px that pushed the overall line height to 20px. Nothing dramatic, but to a keen design eye, it just visibly looks slightly "off".

This PR forces the line height for buttons to match the parent container. For consistency, applying the same style also to .box-footer a elements, even though currently (e.g. in the equivalent browser popup) there's no problem just now. This just makes sure that if in future things change, there won't be a visual regression.

Code changes

  • apps/browser/src/popup/scss/misc.scss and apps/desktop/src/scss/misc.scss: add specific style rule to set explicit line-height: 1

Screenshots

Before - note the slightly uneven line spacing (with the last line being slightly too high)

before screenshot

After - with the line heights evened out

after screenshot

(note that the difference is admittedly very subtle...change from 20px to 17.1429px height on the last line)

Before you submit

  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

patrickhlauke avatar Sep 19 '22 16:09 patrickhlauke

Thank you for your contribution! We've added this to our internal Community PR board for review. ID: PS-1512

bitwarden-bot avatar Sep 19 '22 16:09 bitwarden-bot

@patrickhlauke Could we instead increase the line height of all of these to 20px?

Also, this might not be what you were working on with this but I noticed that the that launches the password history modal/popup doesn't seem to be keyboard focusable currently, do you have any thoughts on how we could improve that?

Screen Shot 2022-09-19 at 12 10 42 PM

jtouchstonebw avatar Sep 19 '22 17:09 jtouchstonebw

@patrickhlauke Could we instead increase the line height of all of these to 20px?

That's a larger design decision that I'll leave up to you/the team. If/when you do that, the line-height: 1 here will adapt the buttons to whatever you set.

Also, this might not be what you were working on with this but I noticed that the that launches the password history modal/popup doesn't seem to be keyboard focusable currently, do you have any thoughts on how we could improve that?

works for me. it's a button that takes focus (that <span> you highlighted is inside the button)

password history number button, with a visible default outline

patrickhlauke avatar Sep 19 '22 17:09 patrickhlauke

@patrickhlauke Could we instead increase the line height of all of these to 20px?

That's a larger design decision that I'll leave up to you/the team. If/when you do that, the line-height: 1 here will adapt the buttons to whatever you set.

Also, this might not be what you were working on with this but I noticed that the that launches the password history modal/popup doesn't seem to be keyboard focusable currently, do you have any thoughts on how we could improve that?

works for me. it's a button that takes focus (that <span> you highlighted is inside the button)

password history number button, with a visible default outline

To clarify, if the row height for each line of text could be the equivalent of 20px so the text is more readable that would be awesome. Thanks!

jtouchstonebw avatar Sep 19 '22 18:09 jtouchstonebw

To clarify, if the row height for each line of text could be the equivalent of 20px so the text is more readable that would be awesome. Thanks!

again, i think that's a larger design decision that should be made globally throughout the app / by design team. i'll leave that out of this PR (but when/if that's done, the change here in the PR will adapt to the new line height)

patrickhlauke avatar Sep 19 '22 18:09 patrickhlauke

but when/if that's done, the change here in the PR will adapt to the new line height)

Gotcha, I understand you now.

jtouchstonebw avatar Sep 19 '22 18:09 jtouchstonebw

my design-obsessed eyes thank you (one of those "once i've seen it, I can't unsee it" things) :)

patrickhlauke avatar Nov 24 '22 18:11 patrickhlauke