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.

Sign up for GitHub

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

Jump to bottom

[dashboard] Don't double-encapsulate Alert contents in <span/> or <p/> #13640

Merged
merged 1 commit into from
Oct 6, 2022
Merged

[dashboard] Don't double-encapsulate Alert contents in <span/> or <p/> #13640

merged 1 commit into from
Oct 6, 2022

Conversation

jankeromnes
Copy link
Contributor

@jankeromnes jankeromnes commented Oct 6, 2022 •
edited
Loading

Description

<Alert> is already a container node. There is no need to double-encapsulate contents inside another container:

<Alert>This is fine</Alert>

<Alert><span>This is unnecessary</span></Alert>

<Alert><div>This too</div></Alert>

<Alert><p>This is actually problematic in dark theme</p></Alert>

Related Issue(s)

Fixes:

Screenshot 2022-10-06 at 09 19 52

How to test

  1. Visit /new and create a user-owned project (i.e. under your Personal Account)
  2. Create a random team (this seems to be required for the Personal Account to show up in the top-left dropdown)
  3. In the dropdown, select your Personal Account
  4. Observe the warning in both light and dark theme

Also:

  • Check other alerts throughout the app

Release Notes

NONE

Documentation

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-payment
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-jx-fix-alerts.1 because the annotations in the pull request description changed
(with .werft/ from main)

@roboquat roboquat added the size/S label Oct 6, 2022
@jankeromnes
Copy link
Contributor Author

jankeromnes commented Oct 6, 2022 •
edited
Loading

Screenshots of the main fix

BEFORE (production) AFTER (this PR)
Screenshot 2022-10-06 at 11 40 36 Screenshot 2022-10-06 at 11 40 03
Screenshot 2022-10-06 at 11 40 24 Screenshot 2022-10-06 at 11 39 51

Other Alerts look unchanged with this PR

DARK LIGHT
Screenshot 2022-10-06 at 11 32 56 Screenshot 2022-10-06 at 11 33 13
Screenshot 2022-10-06 at 11 33 51 Screenshot 2022-10-06 at 11 34 11
Screenshot 2022-10-06 at 11 34 41 Screenshot 2022-10-06 at 11 34 59
Screenshot 2022-10-06 at 11 35 52 Screenshot 2022-10-06 at 11 36 11

@gtsiolis
Copy link
Contributor

gtsiolis commented Oct 6, 2022 •
edited by werft-gitpod-dev-com bot
Loading

Looking at this now! 👀

@jankeromnes jankeromnes requested a review from gtsiolis October 6, 2022 09:48
@jankeromnes jankeromnes marked this pull request as ready for review October 6, 2022 09:49
@jankeromnes jankeromnes requested a review from a team October 6, 2022 09:49
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Oct 6, 2022
gtsiolis
gtsiolis approved these changes Oct 6, 2022
View reviewed changes
Copy link
Contributor

@gtsiolis gtsiolis left a comment

Choose a reason for hiding this comment

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

UX LGTM, @jankeromnes! 🔮

Left one minor copy suggestion.

Thanks for always caring about user experience, how elements render when using the dark theme, including clear steps on how to test the changes, and last but not least adding BEFORE/AFTER screenshots when there are visual changes involved! ✨

Approving, but this will need also an approval from @gitpod-io/engineering-webapp. 🏀

components/dashboard/src/projects/Projects.tsx
</p>
We'll remove projects under personal accounts in Q1'2023.{" "}
<Link to="/new" className="gp-link">
Create a team.
Copy link
Contributor

Choose a reason for hiding this comment

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

🅰️ Minor copy suggestion to follow the pattern of the help links.

Suggested change
Create a team.
Create a team

🅱️ Alternatively, or additionally, we could adopt this new evolving pattern with the → for internal links, see relevant comment in #13414 (comment).

Suggested change
Create a team.
Create a team →

components/dashboard/src/admin/WorkspacesSearch.tsx
<span>Search workspaces using workspace ID.</span>
Search workspaces using workspace ID.
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: Thanks for all the side fixes here, Jan! 🍫

components/dashboard/src/admin/BlockedRepositories.tsx
<span>
Search entries by their repository URL <abbr title="regular expression">RegEx</abbr>.
</span>
Search entries by their repository URL <abbr title="regular expression">RegEx</abbr>.
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: I agree, there's no need to double-encapsulate the contents. The alert component added by @mustard-mh back in https://github.com/gitpod-io/gitpod/pull/8783/files should already handle properly most, if not all, styling use cases, including dark theme colors. 🏓

@roboquat roboquat merged commit 00e04bf into main Oct 6, 2022
@roboquat roboquat deleted the jx/fix-alerts branch October 6, 2022 10:13
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@svenefftinge svenefftinge svenefftinge approved these changes

@gtsiolis gtsiolis gtsiolis approved these changes

Assignees
No one assigned
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/S team: webapp Issue belongs to the WebApp team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jankeromnes @gtsiolis @svenefftinge @roboquat