Add <BaselineStatus> component
#156
Conversation
🦋 Changeset detectedLatest commit: ea56cdd The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
✅ Deploy Preview for astro-embed ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
I tried it out, and it looks quite broken (as you're probably aware).
Screenshot
I've left some suggestions below (unrelated to the broken styling). I'm aware that a lot of these issues stem from the original <baseline-status> widget; their implementation is extremely disappointing. I think it's possible to build something much better. But also, please feel free to ignore these comments; I don't want to overwhelm you 😅
Excited to see where this goes!
| <baseline-status> | ||
| <h1>{feature.name}</h1> | ||
| <details> | ||
| <summary |
There was a problem hiding this comment.
There's definitely some subjectivity here, but I think the <summary> should not have so much content inside it. Most of the content should be outside the <summary>.
I think of it like a <button>, where the general advice is to keep the labels concise.
|
Haha, thanks for the review @mayank99! +1 to all of this. So far I had just literally ripped apart the official component translating it to Astro. (I'm also always a bit unsure of how much people want things that are 1:1 with the official implementation vs diverging but better 😁) Re: styling — last night I was playing around with DSD for isolation, basically: <baseline-status>
<template shadowrootmode="open">
<link rel="stylesheet" href="...">
<!-- component markup -->
</template>
</baseline-status>That's a bit naive I suspect — my first time using DSD — but noted a couple things:
Dark theme support in particular is something I'm thinking about. I was also a bit surprised that they considered |
Good point. It might be worth documenting that the stylesheet should be preloaded if this component is in the critical rendering path. That does mean you'll have to expose the CSS file in the package |
I’m working on an alternative. Remains to be seen if you think it’s a terrible idea. This is my first time really working with Shadow DOM full-stop (let alone DSD), so I could be doing things terribly 😁 Basic idea:
Seems to be working so far. I’m just going through updating the CSS to match this new approach. (So feel free to tell me now why I shouldn’t do this 😅) Edit: one reason is it’s making some stuff difficult, e.g. expressing |
Yeah that's basically the reason. If you can achieve everything you need using You could always inline some of the CSS in |
|
Yeah, that’s basically what I did — only needed to inline two tiny things. Massive commit coming up shortly! |
|
OK, pushed up that big refactor. I’ve also written some docs outlining the hooks for styling: https://deploy-preview-156--astro-embed.netlify.app/components/baseline-status/ Happy to incorporate feedback here as to how this feels and what other custom properties or parts could be useful. Re: Re: custom element namespacing — these have now been removed. There’s a top-level Some outstanding thoughts from your earlier review:
|
For some reason the compiler was generating three `<a>` tags for the link — one additional link after the closing `</template>` and then one more after the final closing `</div>`. These are empty — the text content presumably consumed when outputting the first, correct `<a>`, but can still cause layout issues as well as unnecessary HTML.
The original code used shades that were extremely close to each other for the different Baseline statuses: one for the Baseline checkmark logo, another for the status icons shown next to browsers. The difference seemed more like a mistake than anything and made the color API surface bigger than necessary, so this commit merges those to use the same shades for both.
|
Hmm, what would be the best way for me to test this out locally? |
Ah, good point. I may just publish an early version. That would be easiest for you probably. Otherwise a tool like https://gitpkg.now.sh/ could help to install directly from this branch: npm install 'https://gitpkg.now.sh/delucis/astro-embed/packages/astro-embed-baseline-status?chris/baseline-status'And then: import { BaselineStatus } from '@astro-community/astro-embed-baseline-status';
<BaselineStatus id="has" /> |
|
Going to merge this to release an initial version for testing. We can keep using this PR to discuss if you like or feel free to open a follow-up issue or PR if you prefer. One thing I haven’t done for now is to put these styles in |
|
I got the chance to try it out. Unfortunately it was too limiting to fit my needs.
At this point, I think I'll go with my own implementation so you don't have to worry about any of this. The component works great, and most consumers probably would not run into these issues if they use the component as-is. I also noticed a bug with the layout, which I've opened a PR for. #163 |
|
Thanks for the PR! As mentioned above, happy to move styles to Also totally open to other changes — please don’t feel like it’s a bother! If it’s too limiting for you, it’s potentially too limiting for others and I’d love to improve it. (Also, no evidence yet of anyone else using this, so no reason not to tailor it to your needs 😂) |
Adds a JS-free
<BaselineStatus>component for rendering web feature status. Based on https://github.com/web-platform-dx/baseline-status but without the client-side dependencies.