Skip to content

Add Baseline status to hovercards #428

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

Merged
merged 18 commits into from
Apr 3, 2025
Merged

Conversation

rviscomi
Copy link
Contributor

@rviscomi rviscomi commented Mar 18, 2025

Progress on #427

Hovercard changes:

  • adds baseline icon and status
    • replaces the browser version list
    • icon supports light/dark mode and corresponds to the feature status
  • omits the experimental warning (covered by limited availability status)

Demo:

baseline-vscode.mov
Screenshots...

Widely available

Before:
image

After (dark mode):
image

After (light mode):
image

Newly available

Before:
image

After (dark mode):
image

After (light mode):
image

Limited availability

Before:
image

After (dark mode):
image

After (light mode):
image

Experimental

Before:
image

After (dark mode):
image

After (light mode):
image

(experimental warning removed, as it's redundant with the limited availability message)

Obsolete

Before:
image

After (dark mode):
image

After (light mode):
image

(obsolete warning kept)

@rviscomi rviscomi marked this pull request as ready for review March 19, 2025 20:30
@rviscomi
Copy link
Contributor Author

rviscomi commented Mar 20, 2025

FYI I was getting a TypeScript error related to my use of the Intl.ListFormat function:

$ npm run test

> vscode-css-languageservice@6.3.2 test
> npm run compile && npm run mocha


> vscode-css-languageservice@6.3.2 compile
> tsc -p ./src && npm run copy-jsbeautify && npm run lint

src/languageFacts/entry.ts:193:50 - error TS2339: Property 'ListFormat' does not exist on type 'typeof Intl'.

193 const missingBaselineBrowserFormatter = new Intl.ListFormat("en", {
                                                     ~~~~~~~~~~


Found 1 error in src/languageFacts/entry.ts:193

I can fix it by upgrading the tsconfig to es2021, but figured that would need to be part of a separate discussion. For now I worked around it by changing it to (Intl as any) and left a TODO.

@aeschli
Copy link
Collaborator

aeschli commented Mar 21, 2025

      "browsers": [
        "E12",
        "S10",
        "C50",
        "IE10",
        "O37"
      ],
      "baseline": {
        "status": "high",
        "support": {
          "chrome": "50",
          "chrome_android": "50",
          "edge": "12",
          "firefox": "28",
          "firefox_android": "28",
          "safari": "10",
          "safari_ios": "10"
        },
        "baseline_low_date": "2015-09-30",
        "baseline_high_date": "2018-03-30"
      },
 ```


Now the browser information is duplicated, or have conflicting information? Can't we use the browsers we already have? 
It's ok to add new browsers (just need to spec it)
If it's incomplete or has old data, we can set the data based on the baseline call.

@rviscomi
Copy link
Contributor Author

@aeschli sure I've removed the baseline.support object and added the mobile versions of the Baseline core browser set to the existing browsers array

@rviscomi
Copy link
Contributor Author

Found an issue with the accent-color property, which is not Baseline due to an implementation issue in Safari. In BCD, this is indicated by the partial_implementation flag. compute-baseline takes this into consideration and omits Safari from the list of supported browsers. But Safari still technically has a supported version in BCD and ends up in the list.

Actual:

image

Expected:

image

I fixed this by removing support strings from the browsers array for any core browsers missing from Baseline's support list.

@aeschli
Copy link
Collaborator

aeschli commented Mar 24, 2025

Thanks for the PR. I think we need to improve the look of the hover.

  • I don't find the baseline icon intuitive. I would would go with the ⚠️ and a ✔️
  • I would not put the name first

My suggestion:

  • description
  • compatibility warning (Property is experimental. Property is obsolete, Property is nonstandard)
  • baseline
  • syntax

@rviscomi
Copy link
Contributor Author

Would you be open to moving the Baseline icon next to the status text? It's really the combined power of the icon and availability text (limited, newly, or widely available) that help to clearly convey the feature status.

This would also make the hovercard UI more consistent with other reference docs that talk about compatibility, eg MDN and caniuse:

image image

Mockups

Here's how it could look for the various statuses, taking your other feedback into consideration.

Widely available

image

Newly available

image

Limited availability

image image

@aeschli
Copy link
Collaborator

aeschli commented Mar 24, 2025

The mockups are good. I still don't like the extra wide baseline icons. It's a bit too large for the text size, also I find the form not very intuitive. Is the shape important to you? Can we not just add a checkmark? Or make it a bit smaller?

@aeschli
Copy link
Collaborator

aeschli commented Mar 24, 2025

BTW I adopted the custom data, but didn't make any changes to the hover. Leaving this to you.
https://github.com/microsoft/vscode-css-languageservice/pull/431/files

@rviscomi
Copy link
Contributor Author

Here's an updated mockup of a 10px tall icon instead of the 14px version in light mode:

image

If that looks good to you, I'll push a commit updating the rest of the icons.

@captainbrosset
Copy link

captainbrosset commented Mar 25, 2025

Regarding the use of icons, thank you @aeschli for your feedback on the fact that they're not intuitive. @atopal, let's add this to other feedback we might have received and reflect on whether improvements are needed on the long run.

For now, though (i.e., for this PR), I strongly suggest keeping the Baseline icons as they are defined now, and not deviate from them.
Baseline is a concept that was introduced a couple of years ago and is getting adopted by more and more sites, dashboards, and tools. We pay a lot of attention to how Baseline is being adopted, and sometimes even work hand in hand with consumers to make sure that Baseline is communicated correctly (like now).

The reason is that Baseline is an attempt at summarizing the many details of a very fragmented and nuanced web platform, into something that's as simple as 3 statuses only. This is essentially impossible to do, but we believe that doing it in the way we're doing it now still has more benefits than drawbacks if, and only if, Baseline is communicated correctly.

Changing the icons to a checkmark and a warning icon would simplify what Baseline is even more, to the point that it becomes dangerous.
Indeed, Baseline says "if this is widely available, then you don't really need to worry about it, or test much". That's basically all it says.
Baseline does not say "if this is not widely available, then don't use it". If a feature is not widely available, then it's basically up to the developer to make a conscious decision as to whether they want to use that feature. They may very well use it, even if the feature has limited availability, if they've tested it for their specific use case. Maybe they're using a polyfill in other browsers. Maybe it's part of a progressively enhanced feature set. Who knows.

So, Baseline messages/icons should never appear to developers as something that may steer them away from using a feature.

@aeschli
Copy link
Collaborator

aeschli commented Mar 25, 2025

The smaller baseline icon looks better. Lets go with that!

@rviscomi
Copy link
Contributor Author

Great!

Here are the changes from the latest commit:

  • remove the feature name heading
  • shrink all baseline icons to 10px
  • move the baseline info between the feature description and syntax reference
  • omit the baseline info when there's a non-standard or obsolete feature warning
  • update tests
image image image image

@rviscomi
Copy link
Contributor Author

@aeschli this is ready for another look

@aeschli aeschli added this to the April 2025 milestone Apr 3, 2025
@aeschli aeschli enabled auto-merge (squash) April 3, 2025 14:27
@aeschli
Copy link
Collaborator

aeschli commented Apr 3, 2025

@rviscomi Thanks Rick, nice work, it looks great!

@aeschli aeschli merged commit 0a8ca4c into microsoft:main Apr 3, 2025
3 checks passed
@rviscomi rviscomi deleted the baseline-status branch April 3, 2025 14:40
@rviscomi
Copy link
Contributor Author

rviscomi commented Apr 3, 2025

Thanks for all your help and can't wait to see it live! 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants