-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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: Add a dev overlay #8757
feat: Add a dev overlay #8757
Conversation
🦋 Changeset detectedLatest commit: 8f444a5 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 |
⚖️ Bundle Size CheckLatest commit: 8f444a5
|
Co-authored-by: Sarah Rainsberger <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks really good! Had a few comments and suggestions, but overall very excited for this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving some feedback, but I think you said the aim is to ship and then iterate, so none of these likely need to be blockers, but sharing as I notice for you to tackle when the time is right.
The only things I’d like to see addressed before merging are the individual a11y comments. The bullets below are just general feedback.
Overall, so cool to see this coming together — really amazing work! 👏
-
First impressions: hey, look at that — so cute! ✨
-
At some point a down arrow appears. It wasn’t clear to me when/what interaction triggered this arrow:
-
Clicking the down arrow “hides” the overlay at the bottom of the page:
I’m guessing this state hasn’t been designed yet? Once you get into this state it felt kind of weird to click on for me, because I still recognise it as the full dev overlay, just kind of oddly positioned, so it felt like I might be clicking on the actual buttons rather than bringing the overlay back.
-
Clicking the different buttons selects multiple where I expected them to toggle so one is active at a time (not sure if that assumption is correct, but especially when the Astro icon opens a panel, I expected clicking another button to close the panel):
-
Without docs or having read up about this yet, I don’t know what the cursor arrow and text search (?) icons mean. In the first site I’m trying this on (Starlight starter project) they appear to do nothing, so toggling them on/off feels kind of broken.
-
I then added an island to the page, which made the cursor arrow reveal the island and its props, which worked nicely 🎉
Click-to-open also worked as expected 🙌
-
Read up some of the source code and noticed that the final icon is an “Audit” plugin, so tried adding an image with not
alt
tag to try it out.The red dot highlighting that issues exist is a great touch:
-
On a narrow image the tooltip is pretty tiny, I guess we could afford to add a minimum width?
Co-authored-by: Chris Swithinbank <[email protected]> Co-authored-by: Nate Moore <[email protected]>
It appears on hover after a delay, the time after which is appear is currently mostly a randomly chosen number, probably that a shorter time would make it more clear (also not impossible that it's buggy and the timer refreshes sometimes?)
This is actually the only part of the overlay that matches the supplied design 100% 😅 I'll tell Mark that this felt unclear to you.
I'm still thinking the API on this, but essentially in the future I am planning for us to have a way for a plugin to mark itself as compatible with other plugins, that way it would be possible for an integration to add two plugins that works in tandem (and communicate between them, even) Once that's implemented, all the plugins will be marked as being incompatible with each others by default, so only one of the built-ins will be able to be used at once |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this will be the basis of blog post material, just had some thoughts for beefing up the changeset! See what you think!
packages/astro/src/vite-plugin-dev-overlay/vite-plugin-dev-overlay.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Sarah Rainsberger <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me code-wise! Also played with it locally a bit and it looks great.
My only UX nit is that in a page without islands or a11y issue, when I click the "audit" or "xray" buttons, they didn't do anything and I was confused what their use are. The icons don't really tell much. Showing a tooltip of its use after a long hover over the button would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You addressed all my feedback, looks great! Excited to have this. 🎉
Yep, this is planned! Thank you for reviewing and trying it out. |
packages/astro/src/runtime/client/dev-overlay/plugins/utils/highlight.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your responsiveness and all the great work @Princesseuh — look forward to trying this out more! 🎉
* feat: initial commit for dev overlay * fix: lockfile * fix: build * chore: get ci in a better state * feat: client-server communication * fix: better position for xray * refactor: move icons to separate files * refactor: cleanup components * feat: home screen * refactor: rename icon * feat: flag the feature * fix: cleanup * fix: lockfile * feat: minimize button * Update packages/astro/src/@types/astro.ts Co-authored-by: Sarah Rainsberger <[email protected]> * refactor: cleanup * feat: add ability to go to component for hydrated components * refactor: consistent logic between audit and xray * chore: changeset * Apply suggestions from code review Co-authored-by: Chris Swithinbank <[email protected]> Co-authored-by: Nate Moore <[email protected]> * fix: unchonky the SVGs * fix: button a11y * refactor: move common highlight utilities to a dedicated file * fix: allow tabbing on highlights * fix: allow tooltip clickable sections to be tabbed to * feat: allow using defined icons as plugin icons * refactor: remove unnecessary resolve * Update .changeset/large-stingrays-fry.md Co-authored-by: Sarah Rainsberger <[email protected]> * Update .changeset/large-stingrays-fry.md Co-authored-by: Sarah Rainsberger <[email protected]> * nit: use append * style: small tweaks to minimize button styling --------- Co-authored-by: Sarah Rainsberger <[email protected]> Co-authored-by: Chris Swithinbank <[email protected]> Co-authored-by: Nate Moore <[email protected]>
Changes
This is the first draft of the dev overlay. Not everything is perfect, some parts of the code are quite noticeably incomplete, but, the basis of everything is there.
Testing
Will be done in a separate PR, since this is experimental and very early, that's okay!
Docs
withastro/docs#5183