Add more descriptive tooltip for <= versions #2966

cincodenada · 2021-02-17T20:25:13Z

The existing tooltip can easily be read as implying the feature is only supported in the given range, rather than support starting in that range. It's doubly confusing when the removed version is <=, because the tooltip is written primarily for the added case.

I considered creating an enum rather than 'added'/'removed' magic string values, or just having a boolean e.g. isAdded, so I'm open to suggestions on how to communicate the difference between those cases.

Examples of what this looks like in practice:

Added: :last-child
Added and removed: -webkit-mask-attachment

(Edit: I have now gotten it set up locally and it seems to work as expected 👍)

peterbe · 2021-02-17T21:04:02Z

@cincodenada You did all of that using the GitHub UI?! Cool.
You should really try to check out the project locally. It's easy. At least it should be. And if it isn't, perhaps someone of your caliber can help us improve the README to make it easy/easier to check out the project locally.

@ddbeck or @Elchi3 Can you provide some early/quick feedback to @cincodenada about the underlying feature here? If it helps, I can vet the code and perhaps run it to present some screenshots for an easier discussion.

cincodenada · 2021-02-17T21:26:06Z

@peterbe The first commit was all in the web IDE yeah! It's pretty slick these days. When the linter checks failed I did get it checked out locally so I could run prettier and it was indeed pretty easy to get set up. I reworked the code a bit to avoid a really nasty linebreak in the autoformatting in the slice(), otherwise the formatting fixes were straightforward.

I just checked out the README and it was impressively easy to get up and running, I went ahead and did so, so here's before/after screenshots of -webkit-mask-attachment, my mouse pointer doesn't show but it's hovering over the second ≤37 in both:

cincodenada · 2021-02-17T21:33:35Z

As for mobile tooltips, I know there are various ways to support tooltips on mobile but am not really familiar with them beyond that, and there may be an established way in this or other MDN projects. An alternate solution would be to use something like footnotes, at least for more complicated situations like the <= case.

Also, I'm not committed to any specific wording here, happy to hear feedback there - for instance, I like "Specific version unknown" but I could see it being more verbose than necessary.

(Edit: I went ahead and just changed "Exact" to "Specific", along with some small code tweaks)

cincodenada · 2021-02-17T22:30:20Z

I didn't understand how it worked at first, and in fact got an incorrect idea of how the <= worked from reading the tooltips, which is why I'm here :)

I got some very helpful explanation from some folks from commenting on the browser-compat-data PR that introduced the <= version that I saw, which is how I understand now, it may be helpful context: mdn/browser-compat-data#4637

Of particular note is the "Ranged versions" section of the docs linked above, which explains specifically what <= means: https://github.com/mdn/browser-compat-data/blob/master/schemas/compat-data-schema.md#ranged-versions

cincodenada · 2021-02-18T06:29:03Z

I am not at all criticizing browsers-compact-data here - in fact, my assertion is that as-is the chart is misrepresenting the data they provide. For instance, for both version_removed: "<=37" (meaning support was removed in some unknown version up to or including 37) and version_added: "<=37" (meaning support was added in some unknown version up to or including 37), the tooltip we show says "Supported in version 37 or earlier", which I don't think clearly describes either situation.

The existing code is already calculating that sentence for the tooltip - this PR is just changing the sentence to more accurately reflect the data provided by browsers-compat. And we already take into account all the items in support_structure - we represent notes, flags, prefix, alternative_name, partial implementations, and added/removed versions, each in appropriate ways in the given table. Many of these members are optional and absent in most cases, so having a bunch of empty cells would be a waste of space.

And in any case, the issue at hand is that the sentence we calculate for a specific type of added/removed version (the <= version) doesn't fully describe the meaning that browsers-compat specifies for it, and can be easily misinterpreted so as to not represent the intent of browsers-compat. This PR updates how we calculate the sentence to better reflect the intent of browsers-compat.

Ryuno-Ki · 2021-02-18T08:13:45Z

@cincodenada #2705 is talking about accessible tooltips, so don't worry about it right now.
Thanks for your contribution!

cincodenada · 2021-02-18T08:34:00Z

@Ryuno-Ki Oh, excellent, thanks for the tip! I am also interested in improving tooltips separately, so I appreciate the link :)

peterbe · 2021-02-18T12:38:38Z

@Elchi3 @ddbeck Is this something you have time to review? I'll take a look at the actual code itself but perhaps are (much) better equipped to evaluate the (more important) aspect of the actual functionality change.

ddbeck · 2021-02-18T12:52:28Z

I'm fine with this approach to the title text, though I have a small suggestion for the phrasing (though I'm not sure how best to implement it).

In the screenshot, the final string in the ranged case is: "Specific version unknown: supported ending in version 37 or earlier." That "supported" makes it a little bit awkward to read. "Specific version unknown: support ended in version 37 or earlier." flows a little better.

Alternatively—since there many entries that use this notation—it might be better to put the more specific details first and put the general caveat last. Something like, "Support ended in version 37 or earlier (specific version unknown)"

ddbeck · 2021-02-18T12:53:17Z

By the way, @cincodenada thanks for taking an interest in this. Glad my explanation in the compat repo was helpful!

peterbe

@cincodenada If you can heed Daniel's suggestions I'm ready willing and able to merge this. Great work! We need more people like you.

cincodenada · 2021-02-19T18:00:20Z

Great, thanks for all the feedback! I've updated the wording per @ddbeck's suggestions, which I think are all good improvements.

All good to merge from my end. One final bit of bikeshedding I realized: with the new wording, we could also just use browser-compat-data's wording directly and say "Support added"/"Support removed" (vs started/ended). If that seems markedly better I can make that change as well.

ddbeck · 2021-02-22T12:55:50Z

Great! I don't think we need to exactly follow the wording from BCD. Maybe continue with the wording you've got here and revisit it if there's confusion about the tooltip text?

@peterbe I'm happy with this, but I'll leave actual code review to you.

peterbe

The use of a string as a form of "enum" is a bit strange when there's only ever two possible states. Can you change the signature to:

function labelFromString(
  version: string | boolean | null | undefined,
  added: bool
)

And where you call it, change according to: labelFromString(added, true) or labelFromString(removed, false).

What's cool about TypeScript is that it you give you a nice tooltip in your IDE when you hover over the labelFromString and then it's not so weird to know/see/remember what that second boolean is for.

Another syntax which is quite nice would be something like this:

function labelFromString(
  removed?: string | boolean | null | undefined,
  added?: string | boolean | null | undefined,
)

then you can use it like this, for example:

labelFromString({ removed })

and within the function you'd be able to tell which description to use based on the argument used.

For I think the first suggestion is simpler and more appropriate. A bool instead of a string enum.

cincodenada · 2021-02-27T05:21:41Z

I had considered the boolean option, so I'm happy to go with that - I took a slightly different tack and made it an optional boolean, essentially defaulting to the added case, since removed is the minority/exceptional case. That said, if there's a preference for explicitly including the second argument in both cases, I'm fine with going that way as well.

The property shorthand idea is quite clever indeed, and rather elegant! I also agree it may be just a bit too clever, as much as I love it, and the simpler boolean is the most accessible option 😄

peterbe · 2021-03-01T13:19:36Z

@cincodenada Same problem here. I can't seem to override the fact that your commits aren't signed. Do you mind rebasing your branch locally so all commits are signed?

The existing tooltip can easily be read as implying the feature is only supported in the given range, rather than support starting in that range. It's doubly confusing when the removed version is <=, because then the tooltip is written primarily for the added case.

Before: "Specific version unknown: supported ending in version 37 or earlier." After: "Support ended in version 37 or earlier (specific version unknown)"

Just use a boolean, keep things simple

cincodenada · 2021-04-03T21:32:36Z

Okay, as on #2995 , finally got around to this and rebased/squashed a bit while I was at it. Let me know if there's any remaining feedback, thanks!

Ryuno-Ki

Some nits (but there would be merge conflicts anyway).

Ryuno-Ki · 2021-04-14T15:17:10Z

client/src/document/ingredients/browser-compatibility-table/feature-row.tsx

@@ -101,14 +101,19 @@ function NonBreakingSpace() {
  return <>{"\u00A0"}</>;
 }

-function labelFromString(version: string | boolean | null | undefined) {
+function labelFromString(
+  version: string | boolean | null | undefined,


Ryuno-Ki · 2021-04-14T15:18:57Z

client/src/document/ingredients/browser-compatibility-table/feature-row.tsx

  if (typeof version !== "string") {
    return <>{"?"}</>;
  }
  if (!version.startsWith("≤")) {
    return <>{version}</>;
  }
-  const title = `Supported in version ${version.slice(1)} or earlier.`;
+  const actionDescription = removed ? "ended" : "started";
+  const versionDescription = `version ${version.slice(1)} or earlier`;


Assuming version has a value of ≤ 79 there would be a doubled space here.
Is it guaranteed to be in shape of ≤79?
(79 being a random number)

Ryuno-Ki · 2021-04-14T15:20:41Z

client/src/document/ingredients/browser-compatibility-table/feature-row.tsx

-  const title = `Supported in version ${version.slice(1)} or earlier.`;
+  const actionDescription = removed ? "ended" : "started";
+  const versionDescription = `version ${version.slice(1)} or earlier`;
+  const title = `Support ${actionDescription} in ${versionDescription} (specific version unknown)`;
  return (
    <span title={title}>
      <sup>≤&#xA0;</sup>


Suggested change

<sup>≤ </sup>

<sup>≤<NonBreakingSpace /></sup>

(for readability)

Elchi3 · 2021-04-14T17:12:24Z

In #3238 a proposal was made to hide the "≤" annotation altogether to avoid confusion and we came to agreement there, so I think this PR should be closed. Thanks for your work, though!

escattone · 2021-04-15T20:35:14Z

Closing, but thanks for all of your work on this @cincodenada!

cincodenada · 2021-04-15T21:55:37Z

Oh, yeah reading up on #3238 I agree that is the better solution overall, the <= doesn't really add much information in this context, so if we can just clear up the confusion by leaving it out I'm all for it!

Thanks for looping back here and for all the feedback in the process, even though it ended up being superceded :)

Elchi3 · 2021-04-16T06:08:05Z

Thanks @cincodenada! Your work definitely also helped us to realize what is best 👍

mdn deleted a comment Feb 17, 2021

mdn blocked ghost Feb 18, 2021

peterbe approved these changes Feb 18, 2021

View reviewed changes

cincodenada mentioned this pull request Feb 19, 2021

Add release date for support statements with multiple versions #2995

Merged

peterbe suggested changes Feb 22, 2021

View reviewed changes

cincodenada force-pushed the patch-2 branch from 32b07e2 to de8b101 Compare February 27, 2021 05:19

peterbe approved these changes Mar 1, 2021

View reviewed changes

cincodenada added 3 commits April 3, 2021 14:31

Improve wording per PR feedback

9a93595

Before: "Specific version unknown: supported ending in version 37 or earlier." After: "Support ended in version 37 or earlier (specific version unknown)"

Simplify function signature

beea22e

Just use a boolean, keep things simple

cincodenada force-pushed the patch-2 branch from de8b101 to beea22e Compare April 3, 2021 21:32

Ryuno-Ki reviewed Apr 14, 2021

View reviewed changes

escattone closed this Apr 15, 2021

caugner added the browser-compat issues related to the browser compatibility data tables (BCD) label Apr 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more descriptive tooltip for <= versions #2966

Add more descriptive tooltip for <= versions #2966

cincodenada commented Feb 17, 2021 •

edited

Loading

peterbe commented Feb 17, 2021

cincodenada commented Feb 17, 2021 •

edited

Loading

cincodenada commented Feb 17, 2021 •

edited

Loading

cincodenada commented Feb 17, 2021

cincodenada commented Feb 18, 2021

Ryuno-Ki commented Feb 18, 2021

cincodenada commented Feb 18, 2021

peterbe commented Feb 18, 2021

ddbeck commented Feb 18, 2021

ddbeck commented Feb 18, 2021

peterbe left a comment

cincodenada commented Feb 19, 2021

ddbeck commented Feb 22, 2021

peterbe left a comment

cincodenada commented Feb 27, 2021 •

edited

Loading

peterbe commented Mar 1, 2021

cincodenada commented Apr 3, 2021 •

edited

Loading

Ryuno-Ki left a comment

Ryuno-Ki Apr 14, 2021

Ryuno-Ki Apr 14, 2021

Ryuno-Ki Apr 14, 2021

Elchi3 commented Apr 14, 2021

escattone commented Apr 15, 2021

cincodenada commented Apr 15, 2021 •

edited

Loading

Elchi3 commented Apr 16, 2021

	version: string \| boolean \| null \| undefined,
	version?: string \| boolean \| null,

Add more descriptive tooltip for <= versions #2966

Add more descriptive tooltip for <= versions #2966

Conversation

cincodenada commented Feb 17, 2021 • edited Loading

peterbe commented Feb 17, 2021

cincodenada commented Feb 17, 2021 • edited Loading

cincodenada commented Feb 17, 2021 • edited Loading

cincodenada commented Feb 17, 2021

cincodenada commented Feb 18, 2021

Ryuno-Ki commented Feb 18, 2021

cincodenada commented Feb 18, 2021

peterbe commented Feb 18, 2021

ddbeck commented Feb 18, 2021

ddbeck commented Feb 18, 2021

peterbe left a comment

Choose a reason for hiding this comment

cincodenada commented Feb 19, 2021

ddbeck commented Feb 22, 2021

peterbe left a comment

Choose a reason for hiding this comment

cincodenada commented Feb 27, 2021 • edited Loading

peterbe commented Mar 1, 2021

cincodenada commented Apr 3, 2021 • edited Loading

Ryuno-Ki left a comment

Choose a reason for hiding this comment

Ryuno-Ki Apr 14, 2021

Choose a reason for hiding this comment

Ryuno-Ki Apr 14, 2021

Choose a reason for hiding this comment

Ryuno-Ki Apr 14, 2021

Choose a reason for hiding this comment

Elchi3 commented Apr 14, 2021

escattone commented Apr 15, 2021

cincodenada commented Apr 15, 2021 • edited Loading

Elchi3 commented Apr 16, 2021

cincodenada commented Feb 17, 2021 •

edited

Loading

cincodenada commented Feb 17, 2021 •

edited

Loading

cincodenada commented Feb 17, 2021 •

edited

Loading

cincodenada commented Feb 27, 2021 •

edited

Loading

cincodenada commented Apr 3, 2021 •

edited

Loading

cincodenada commented Apr 15, 2021 •

edited

Loading