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

feat(audits): Add new UI #10217

Merged
merged 24 commits into from Mar 8, 2024
Merged

feat(audits): Add new UI #10217

merged 24 commits into from Mar 8, 2024

Conversation

Princesseuh
Copy link
Member

@Princesseuh Princesseuh commented Feb 23, 2024

Changes

This reworks the Audit UI in the top right to generally rework the visual aspect, and also add a detailed view

List View:

image

Detailed View:

image

No audits:

image

Fix #10327

Testing

Tests should pass! We don't test every single interaction between the cards and the highlights, nonetheless we still test that everything shows as expected.

Docs

N/A. In the future, this will allow for cool long descriptions for every audits, which will need a fair amount of docs work, similar to the work on errors.

Copy link

changeset-bot bot commented Feb 23, 2024

🦋 Changeset detected

Latest commit: 7a9f1d7

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Feb 23, 2024
@github-actions github-actions bot added the semver: minor Change triggers a `minor` release label Feb 23, 2024
github-actions[bot]

This comment was marked as outdated.

@Princesseuh

This comment was marked as outdated.

This comment was marked as outdated.

@Princesseuh Princesseuh added this to the 4.5.0 milestone Mar 4, 2024
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

The tiniest nit, but makes a review worth it 😹

.changeset/witty-wombats-attend.md Outdated Show resolved Hide resolved
Comment on lines 18 to 22
}

connectedCallback() {
const classes = [`badge--${this.size}`, `badge--${this.badgeStyle}`];
this.shadowRoot.innerHTML = `
this.shadowRoot.innerHTML += `
Copy link
Member Author

Choose a reason for hiding this comment

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

This change will be removed by #10186, but it has the same effect

@@ -159,6 +159,7 @@ export class AstroDevToolbar extends HTMLElement {
opacity: 0;
transition: opacity 0.2s ease-in-out 0s;
pointer-events: none;
user-select: none;
Copy link
Member Author

Choose a reason for hiding this comment

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

Just a small thing I noticed while working on this, it's technically possible for the tooltip on hover to get selected by users somehow.

@Princesseuh Princesseuh marked this pull request as ready for review March 7, 2024 19:09
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

Consider this design approval on the screenshots you shared! Have not looked at the code, but I'm sure it's #NWTWWHB.

@bluwy
Copy link
Member

bluwy commented Mar 8, 2024

It looks like it's not rendered right on Safari (I'm using 17.4) (works well on Firefox and Chrome):

Screenshot image image

Seems like a fixed width needs to be set on the astro-dev-toolbar-icon to prevent it from occupying the flex space, since the inner SVG element has width: 100%.


Also, would be nice to resolve the eslint warnings related to the dev toolbar 😬 https://github.com/withastro/astro/actions/runs/8193520483/job/22407514922?pr=10217#step:8:26

@Princesseuh
Copy link
Member Author

I forgot to test Safari 😔 Will fix! Thank you for checking

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Tested it on a real project with some audits, and the changes seem good to me :)

@Princesseuh
Copy link
Member Author

Fixed Safari styling! As for the lints, they'll be fixed once we merge #10186 in, because it requires adding an error level to the logger, which that PR adds.

@Princesseuh Princesseuh merged commit 5c7862a into main Mar 8, 2024
7 of 13 checks passed
@Princesseuh Princesseuh deleted the feat/cooler-audits branch March 8, 2024 11:10
@astrobot-houston astrobot-houston mentioned this pull request Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected slow initial render when using "visibility: hidden" and "<label>"
6 participants