Code Feedback

[jcscottiii]

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:

• https://github.com/GoogleChrome/baseline-status/blob/e5affba03866e1a36f4e43a675da53dc98fe24b5/baseline-status.js#L229
  • status could be null.
• https://github.com/GoogleChrome/baseline-status/blob/e5affba03866e1a36f4e43a675da53dc98fe24b5/baseline-status.js#L242-L245
  • The current formation of the description and prefix is a little unclear at first glance. Especially with the lowercase t and finding out where it is appended. You could have a method called getBaselineDescription(baseline_status)
• https://github.com/GoogleChrome/baseline-status/blob/e5affba03866e1a36f4e43a675da53dc98fe24b5/baseline-status.js#L218-L220
  • support is reassigned twice. Instead, create a variable called support_class_suffix that is computed like you currently have it. Add a comment that for newly available baseline features or no data, we render the same icon across all browsers. Otherwise, render browser specific details. You should also rename support to browser_support since it reads from browser_implementation. Additionally, the values for browser_implementation are "available" and "unavailable" not "widely" or "limited".
  • Also for widely, you can just render widely which comes from baseline as well instead of the browser specific ones.

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.

  • import limitedSvg from './assets/icons/limited.svg'; 
    ...
    ... 
    
    export default class BaselineIcon extends LitElement {
      static get styles() {
        return css`
          :host {
          --baseline-icon-limited: url(${limitedSvg});
    

Style:

• Leverage GTS for the style guide. It can be used with eslint to enforce and prettier to reformat.

Typing:

• To help with the responses and preventing yourself from having to manually check the possible values, you could generate the types based on the openapi document for the backend. Then use type hinting annotations to help. This will help with the maps between data and css classes as well as reduce any other potential bugs (like the values for browser implementation). You can use something like this to generate the typing based on the openapi.yaml: https://www.npmjs.com/package/openapi-typescript

Error handling:

• In the event, there's a 5xx error, you may want to render a retry button or a message to try again later.

[devnook]

Thanks a lot, James!

Re:

• Code - Please take a look: Code updates according to https://github.com/GoogleChrome/baseline-status/issues/3 #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 Improve accessibility for users of assistive technologies #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 Add typescript support and openapi integration with backend #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.

[jcscottiii]

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 🎉! 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 😅 )

[devnook]

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?

[jcscottiii]

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

[atopal]

@devnook re errors, in the PRD we said to not render the widget if the data is missing. I don't have strong opinions on that though. In terms of changing the visual design of the widget, please file an issue here and cc Mustafa and/or add a note for him in Figma.