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 <sarah@rainsberger.ca>
natemoo-re
left a comment
There was a problem hiding this comment.
Code looks really good! Had a few comments and suggestions, but overall very excited for this!
delucis
left a comment
There was a problem hiding this comment.
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
alttag 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 <swithinbank@gmail.com> Co-authored-by: Nate Moore <natemoo-re@users.noreply.github.com>
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 |
sarah11918
left a comment
There was a problem hiding this comment.
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 <sarah@rainsberger.ca>
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
bluwy
left a comment
There was a problem hiding this comment.
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.
natemoo-re
left a comment
There was a problem hiding this comment.
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
delucis
left a comment
There was a problem hiding this comment.
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 <sarah@rainsberger.ca> * 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 <swithinbank@gmail.com> Co-authored-by: Nate Moore <natemoo-re@users.noreply.github.com> * 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 <sarah@rainsberger.ca> * Update .changeset/large-stingrays-fry.md Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca> * nit: use append * style: small tweaks to minimize button styling --------- Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca> Co-authored-by: Chris Swithinbank <swithinbank@gmail.com> Co-authored-by: Nate Moore <natemoo-re@users.noreply.github.com>
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