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

Even more responsive notification bar #2151

Merged
merged 3 commits into from
Nov 4, 2021
Merged

Conversation

djsmith85
Copy link
Contributor

Type of change

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

Objective

Remove the handling of different window sizes, with modifying text and styles from bar.js and instead do everything with CSS.

Asana ticket: https://app.asana.com/0/1169444489336079/1200912318004726/f

Code changes

  • src/notification/bar.html:

    • Added html attributes short-text and full-text, added an attribute to contain isVaultLocked state
  • src/notification/bar.js:

    • Removed the logic to differentiate between window sizes and setting textContent with different translations
    • Fill new attributes short-text and full-text with the translations pulled from i18n
    • Pass isVaultLocked value as a string into the isVaultLocked attribute of the select-folder dropdown
  • src/notification/bar.scss:

    • Added media queries to adjust the translations by extracting the attribute value of either short-text or full-text
    • Hide select folder dropdown on smaller screens
    • Hide the select folder dropdown if the isVaultLocked attribute has the value set to "true"

Testing requirements

  • Add new login

    • The message "Should Bitwarden remember this password for you?" should always be readable
    • With a window width smaller than 768px
      • Never
      • Yes
      • X
    • With a window width wider than 768px
      • Never for this site
      • Select folder dropdown if vault is unlocked, hidden if locked
      • Yes, Save Now
      • X
  • Change password

    • The message "Do you want to update this password in Bitwarden?" should always be readable
    • With a window width smaller than 768px
      • Yes
      • X
    • With a window width wider than 768px
      • Yes, Update Now
      • X

Before you submit

  • I have checked for linting errors (npm run lint) (required)
  • This change requires a documentation update (notify the documentation team)
  • This change has particular deployment requirements (notify the DevOps team)

@djsmith85 djsmith85 requested a review from eliykat October 28, 2021 21:46
Copy link
Member

@eliykat eliykat left a comment

Choose a reason for hiding this comment

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

I think this is a really simple and succinct solution. However I'm concerned that using ::after pseudo-classes like this is not accessible. See https://accessibleweb.com/question-answer/why-cant-i-use-css-before-and-after-to-insert-content/.

To be honest, I think the long form text is unnecessary. Yes/No seem like clear answers, and if they're not, then the question should probably be phrased differently. I'd suggest checking with Danielle whether we can just use a single set of UI text.

However, if that's not possible, then I'd recommend using the techniques here to listen to a change in window size in the javascript, and set the UI text accordingly. That would be accessible and would basically emulate CSS media queries.

@danielleflinn
Copy link
Member

Yes we can use the same language for the buttons regardless of screen size. Let's use the following copy/buttons:

Save Password:

  • Question: "Do you want Bitwarden to save this password?"
  • Never / Save

Update Password:

  • Question: "Do you want Bitwarden to update this password?"
  • Never / Update

@djsmith85 djsmith85 requested a review from eliykat November 4, 2021 17:45
Copy link
Member

@eliykat eliykat left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this @djsmith85!

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