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

[PS-1388] Desktop, browser, web: Don't prevent whitespace wrapping in links/buttons, widen desktop pages #3407

Merged
merged 5 commits into from
Sep 6, 2022

Conversation

patrickhlauke
Copy link
Contributor

@patrickhlauke patrickhlauke commented Aug 28, 2022

Type of change

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

Objective

Allowing whitespace to wrap solves the issue of long link/button text awkwardly breaking out of controls
Widening desktop "pages" prevents some unnecessary wrapping in places like the "Create account" button on the login screen, whose content is slightly wider than it should be (but this is currently masked by the nowrap)

Closes #2620

Code changes

  • apps/browser/src/popup/scss/base.scss, apps/browser/src/popup/scss/buttons.scss, apps/desktop/src/scss/base.scss, apps/desktop/src/scss/buttons.scss, apps/web/src/scss/buttons.scss: remove white-space: nowrap from the various button/link styles, allowing overly-long content to wrap rather than break out
  • apps/desktop/src/scss/pages.scss: bump the width of desktop pages from 300px to 325px (admittedly, this makes them go out of sync with small modals?)

Screenshots

Before (taken from #2620):

screenshot of desktop login, with the SSO button text escaping the button

After:

screenshot of the desktop login, slightly wider by 25px, but with the SSO button text wrapping over two lines instead of breaking out

I had a look around the desktop, web, and popup versions with this change applied, and did not come across any other changes/adverse effects - though recommend giving them a thorough once-over, just in case some situation/screen did rely on the nowrap

Before you submit

- [x] I have checked for **linting** errors (`npm run lint`) (required)
- [ ] I have added **unit tests** where it makes sense to do so (encouraged but not required)
- [ ] This change requires a **documentation update** (notify the documentation team)
- [ ] This change has particular **deployment requirements** (notify the DevOps team)

Allowing whitespace to wrap solves the issue of long link/button text awkwardly breaking out of controls
Widening desktop "pages" prevents some unnecessary wrapping in places like the "Create account" button on the login screen, whose content is slightly wider than it should be (but this is currently masked by the `nowrap`)

Closes bitwarden#2620
@patrickhlauke patrickhlauke changed the title Don't prevent whitespace wrapping in links/buttons, widen desktop pages Desktop, browser, web: Don't prevent whitespace wrapping in links/buttons, widen desktop pages Aug 28, 2022
@bitwarden-bot
Copy link

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

@bitwarden-bot bitwarden-bot changed the title Desktop, browser, web: Don't prevent whitespace wrapping in links/buttons, widen desktop pages [PS-1388] Desktop, browser, web: Don't prevent whitespace wrapping in links/buttons, widen desktop pages Aug 28, 2022
@patrickhlauke
Copy link
Contributor Author

/cc @danielleflinn as this affects layout/UI

@patrickhlauke
Copy link
Contributor Author

patrickhlauke commented Aug 29, 2022

Just noticed #2714 which tries to tackle the issue from the other end (setting white-space: normal in specific instances). Not sure which way would be best ... personally, I don't like setting something and then unsetting it in specific case, but rather avoiding the problem altogether at source (not setting white-space: nowrap).

@danielleflinn
Copy link
Member

Thanks Patrick! We recently created a new Github team for design changes. If you can use @bitwarden/dept-design in future PRs that affect UI that would be much appreciated.

As always, thank you for your contribution!

@patrickhlauke
Copy link
Contributor Author

@danielleflinn sure thing!

@patrickhlauke
Copy link
Contributor Author

@danielleflinn https://github.com/orgs/bitwarden/teams/dept-design 404s for me, and when I try to manually @bitwarden/dept-design it doesn't seem to recognise it ... is it publicly accessible?

@danielleflinn
Copy link
Member

Oh you are right; looks like only Github organization members can tag a team. Feel free to ignore the previous comment then.

@djsmith85 djsmith85 requested a review from a team August 29, 2022 17:33
Copy link
Contributor

@djsmith85 djsmith85 left a comment

Choose a reason for hiding this comment

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

This is looking good. @patrickhlauke Thank you for your contributions.

@djsmith85 djsmith85 merged commit 55e5e00 into bitwarden:master Sep 6, 2022
@patrickhlauke patrickhlauke deleted the patrickhlauke-issue2620 branch May 28, 2023 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

the Italian SSO button label comes out of the edges
4 participants