Open jcscottiii opened 4 months ago
Thanks a lot, James!
Re:
Code - Please take a look: https://github.com/GoogleChrome/baseline-status/pull/4
SVGs: I was going back and forth on this multiple times. The problem is, you cannot import svgs in js modules that easily:
The code needs to be bundled for this to work, and most bundlers need a plugin to import svgs to js. E.g. here I am using rollup and would need something like rollup-plugin-svg-import
.
The general advice for web components using Lit is to distribute the code unbundled, so that the project using the web component can bundle it its own way (and eg use tree shaking to remove multiple instances of Lit etc). I am worried extracting SVGs would add extra hurdle for folks who want to install this component, without that much added value on our side.
GTS: in-progress in Tom's PR https://github.com/GoogleChrome/baseline-status/pull/2
Typing - I'll add this as nice to have. I plan to rewrite it to typescript anyways so this will be a good moment to look at openapi too. Logged at https://github.com/GoogleChrome/baseline-status/issues/5
Error handling: As this is not an interactive component, in case of a server error we just display "data unavailable" message according to the design.
No problem!
I added some comments on #4
SVGs: We can accomplish it with image plugin with set to dom=false from the rollup official plugins. I have added a draft PR (#7) to help get started. (I know there are lot of pending changes happening right now so I didn't want to get too far in the rabbit hole). Feel free to take a look. I will say the BaselineIcon component won't be able to use it without some refactoring. It will complain about unsafeCSS because the svg is used directly in the CSS. We shouldn't use unsafeCSS either though. But the rest of the code can use it.
GTS: I am happy to hear that GTS will be used :tada:! I think the change has been reverted for now. But looking forward to it getting setup and added to the package.json
Typing: You're right. It makes sense to defer using that package I suggested until we actually are using TypeScript. I added some type definitions in my comment on #4 to help in the meantime.
Error handling: I see we only have simple interactions for now (like the drop down). And for now, we can definitely go with your suggestion. But maybe we could reconsider in the future? (But also, hopefully the API is stable enough and we won't need this in the future anyhow :sweat_smile: )
SVGs: Thanks for the demo. It shows exactly the problem I described earlier - if we distribute the unbundled component, as per Lit recommendation, the developer using the component in their project would need to go through the extra hoops changing their build workflow to add SVGs bundling. My assessment is that the gain of having svgs in an external file is lower than the additional hurdle caused to the developer. Therefore, I'd like to keep the SVGs inline as it is now.
GTS: It's still there, just waiting to be merged to avoid massive merge conflict.
Types: Cool, we can use those.
Errors: Requests for change of look&feel or functionality should go through @atopal as this project has mutiple stakeholders agreeing on the final design. @atopal what would be the best way to request changes to the current design? Would an issue on this repo be a good place?
SVGs: Thanks for the demo. It shows exactly the problem I described earlier - if we distribute the unbundled component, as per Lit recommendation, the developer using the component in their project would need to go through the extra hoops changing their build workflow to add SVGs bundling. My assessment is that the gain of having svgs in an external file is lower than the additional hurdle caused to the developer. Therefore, I'd like to keep the SVGs inline as it is now.
Whoops I missed your point the first time around. And I see what you are saying now Docs
We could have a separate rollup config that only does inlining but we can look into that later. With that kind of setup, it would be helpful for external contributors that want to easily update logos
Here's my first review of things right before I log off. (So let me know if something needs more clarification)
Overview: The code fetches the status from the API 🎉 ! And it handles standard and dark mode rendering of the baseline icon. Also, I appreciate the snapshot tests. That will help with maintainability.
Here are some areas of improvement:
Code:
t
and finding out where it is appended. You could have a method calledgetBaselineDescription(baseline_status)
browser_support
since it reads from browser_implementation. Additionally, the values for browser_implementation are "available" and "unavailable" not "widely" or "limited".SVGs
You can move each icon to a separate SVG file (maybe in a directory called
assets
). Then import them. This will help with the organization of the icons.export default class BaselineIcon extends LitElement { static get styles() { return css` :host { --baseline-icon-limited: url(${limitedSvg});
Style:
Typing:
Error handling: