Code updates according to https://github.com/GoogleChrome/baseline-status/issues/3 #4

devnook · 2024-05-31T08:02:58Z

No description provided.

tomayac · 2024-05-31T10:40:32Z

This will cause conflicts with #2. Please merge #2 first, and then update #4 based on the resulting init-branch.

devnook · 2024-05-31T11:23:05Z

@tomayac This is main reason why I am against unannounced, all-encompassing formatting changes in an unrelated PR. Merging these 2 PRs will be quite some work.

tomayac · 2024-05-31T11:35:31Z

Happy to deal with this. If you merge #2, I'll merge #4. Deal?

devnook · 2024-05-31T11:44:37Z

Please wait for @jcscottiii review first.

jcscottiii

You can add some JSDocs to the top near ICONS to help out with this too

/**
 * @typedef {Object} BrowserImplementation
 * @property {'available' | 'not_available'} status
 */

/**
 * @typedef {Object} BaselineData
 * @property {'limited' | 'newly' | 'widely' | 'no_data'} status
 * @property {string} [low_date]
 * @property {string} [high_date]
 */

/**
 * @typedef {Object} Feature
 * @property {string} name
 * @property {string} feature_id 
 * @property {BaselineData} baseline
 * @property {{ chrome?: BrowserImplementation, edge?: BrowserImplementation, firefox?: BrowserImplementation, safari?: BrowserImplementation }} [browser_implementations]
 */

jcscottiii · 2024-05-31T14:48:16Z

baseline-status.js

    return html`<browser-support-icon class="support-${support}">
        <svg xmlns="http://www.w3.org/2000/svg" width="17" height="21" fill="none" viewBox="0 0 17 21"><path fill="currentColor" d="M1.253 3.31a8.843 8.843 0 0 1 5.47-1.882c4.882 0 8.838 3.927 8.838 8.772 0 4.845-3.956 8.772-8.837 8.772a8.842 8.842 0 0 1-5.47-1.882c-.237.335-.49.657-.758.966a10.074 10.074 0 0 0 6.228 2.14c5.562 0 10.07-4.475 10.07-9.996 0-5.52-4.508-9.996-10.07-9.996-2.352 0-4.514.8-6.228 2.14.268.309.521.631.757.966Z"/><path fill="currentColor" d="M11.348 8.125 6.34 13.056l-3.006-2.954 1.002-.985 1.999 1.965 4.012-3.942 1.002.985Z"/></svg>
        <svg xmlns="http://www.w3.org/2000/svg" width="17" height="21" fill="none" viewBox="0 0 17 21"><path fill="currentColor" d="M1.254 3.31a8.843 8.843 0 0 1 5.47-1.882c4.881 0 8.838 3.927 8.838 8.772 0 4.845-3.957 8.772-8.838 8.772a8.842 8.842 0 0 1-5.47-1.882c-.236.335-.49.657-.757.966a10.074 10.074 0 0 0 6.227 2.14c5.562 0 10.071-4.475 10.071-9.996 0-5.52-4.509-9.996-10.07-9.996-2.352 0-4.515.8-6.228 2.14.268.309.52.631.757.966Z"/><path fill="currentColor" d="m10.321 8.126-1.987 1.972 1.987 1.972-.993.986-1.987-1.972-1.987 1.972-.993-.986 1.986-1.972-1.986-1.972.993-.986 1.987 1.972L9.328 7.14l.993.986Z"/></svg>
      </browser-support-icon>`;
  }

  renderTemplate(feature, isLoading) {
-    const baseline = feature.baseline.status;
-    let prefix = '';
+    const baseline = feature.baseline.status || '';


You should make the default value your no_data placeholder. BASELINE_DEFS does not have an entry for ''

jcscottiii · 2024-05-31T15:13:38Z

baseline-status.js

-    support = (baseline === 'newly' || baseline === 'no_data') ? baseline : support;
-
+  renderSupportIcon(baseline, browserImplementation) {
+    const isSupported = browserImplementation?.status === 'available';


Nit:

There are two cases where we use the support-widely class:

When the baseline status == widely. (Which means the feature has been supported on all browsers for an extended period of time, or

When the baseline status == limited but it is supported in a given browser.

It may be confusing to developers that we are using the widely class for case 2 when we just want the color.

We should create a separate css class suffix called something like browser-available. Therefore you would add these lines to the styles above:

browser-support-icon.support-browser-available svg:last-child { display: none; } .support-browser-available { color: var(--color-widely); }

(you could go further and create a separate --color-browser-available definition too. But that can always come later if we want to have a different color for the browser specific availability)

With this renderSupportIcon could look like this:

/** * Determine the support class for the browser support icon based on the feature's baseline status and browser implementation. * * @param {('limited'|'newly'|'widely'|'no_data')} baseline - The baseline status of the feature (including the 'no_data' state, which represents unknown/loading). * @param {object|undefined} browserImplementation - The browser implementation details. Only used if `baseline` is 'limited', indicating that not all browsers support the feature and we need to display browser-specific support information. */ renderSupportIcon(baseline, browserImplementation) { let support = baseline; // Special handling for the 'limited' baseline status if (baseline === 'limited') { support = browserImplementation?.status === 'available' ? 'browser-available' : 'limited'; } return html`<browser-support-icon class="support-${support}"> <svg xmlns="http://www.w3.org/2000/svg" width="17" height="21" fill="none" viewBox="0 0 17 21"><path fill="currentColor" d="M1.253 3.31a8.843 8.843 0 0 1 5.47-1.882c4.882 0 8.838 3.927 8.838 8.772 0 4.845-3.956 8.772-8.837 8.772a8.842 8.842 0 0 1-5.47-1.882c-.237.335-.49.657-.758.966a10.074 10.074 0 0 0 6.228 2.14c5.562 0 10.07-4.475 10.07-9.996 0-5.52-4.508-9.996-10.07-9.996-2.352 0-4.514.8-6.228 2.14.268.309.521.631.757.966Z"/><path fill="currentColor" d="M11.348 8.125 6.34 13.056l-3.006-2.954 1.002-.985 1.999 1.965 4.012-3.942 1.002.985Z"/></svg> <svg xmlns="http://www.w3.org/2000/svg" width="17" height="21" fill="none" viewBox="0 0 17 21"><path fill="currentColor" d="M1.254 3.31a8.843 8.843 0 0 1 5.47-1.882c4.881 0 8.838 3.927 8.838 8.772 0 4.845-3.957 8.772-8.838 8.772a8.842 8.842 0 0 1-5.47-1.882c-.236.335-.49.657-.757.966a10.074 10.074 0 0 0 6.227 2.14c5.562 0 10.071-4.475 10.071-9.996 0-5.52-4.509-9.996-10.07-9.996-2.352 0-4.515.8-6.228 2.14.268.309.52.631.757.966Z"/><path fill="currentColor" d="m10.321 8.126-1.987 1.972 1.987 1.972-.993.986-1.987-1.972-1.987 1.972-.993-.986 1.986-1.972-1.986-1.972.993-.986 1.987 1.972L9.328 7.14l.993.986Z"/></svg> </browser-support-icon>`; }

jcscottiii · 2024-05-31T15:55:51Z

baseline-status.js

@@ -24,7 +24,7 @@ const BASELINE_DEFS = {
  },
  'newly': {
    title: '',
-    description: 'his feature works across the latest devices and browser versions. This feature might not work in older devices or browsers.'
+    description: 'This feature works across the latest devices and browser versions. This feature might not work in older devices or browsers.'


This description will never be used since a newly feature will also have the low_date and your code below overrides the description with string Since...

Also I just realized that the string for widely is hardcoded with a date of September 2021. Here's an example feature: Set. Set first hit baseline in 2015. It may be weird to say September 2021. We should use the low_date for that widely too.

With those points in mind, we may want to use a new method called getFeatureDescription. Using the jsdoc typedef I mentioned in the review comment.

/** * Gets the formatted feature description. * * @param {Feature} feature - The feature data object. * @returns {string} The formatted feature description. */ getFeatureDescription(feature) { if (feature.baseline.status === 'newly' && feature.baseline.low_date) { const formattedDate = new Intl.DateTimeFormat('en-US', { year: 'numeric', month: 'long' }).format(new Date(feature.baseline.low_date)); return `Since ${formattedDate}, this feature works across the latest devices and browser versions. This feature might not work in older devices or browsers.`;expand_more } else if (feature.baseline.status === 'widely' && feature.baseline.low_date) { // Do similar formating } else if (feature.baseline.status === 'limited') { // return your constant string } // Some catch-all default for unknown }

devnook · 2024-06-04T08:53:16Z

You can add some JSDocs to the top near ICONS to help out with this too

/**
 * @typedef {Object} BrowserImplementation
 * @property {'available' | 'not_available'} status
 */

/**
 * @typedef {Object} BaselineData
 * @property {'limited' | 'newly' | 'widely' | 'no_data'} status
 * @property {string} [low_date]
 * @property {string} [high_date]
 */

/**
 * @typedef {Object} Feature
 * @property {string} name
 * @property {string} feature_id 
 * @property {BaselineData} baseline
 * @property {{ chrome?: BrowserImplementation, edge?: BrowserImplementation, firefox?: BrowserImplementation, safari?: BrowserImplementation }} [browser_implementations]
 */

Tracked in #5

…navailable

devnook · 2024-06-04T08:55:45Z

@jcscottiii PTAL

jcscottiii

LGTM with one suggestion

jcscottiii · 2024-06-04T18:10:30Z

baseline-status.js

+      '';
+    const baselineDate = this.getBaselineDate(feature);
+    const description = this.getDescriptionDate(baseline, baselineDate);
+    const year = baseline === 'newly' ? baselineDate.split(' ')[1] : '';


To be consistent, we should check baselineDate like we do in getDescriptionDate

Suggested change

const year = baseline === 'newly' ? baselineDate.split(' ')[1] : '';

const year = baseline === 'newly' && baselineDate ? baselineDate.split(' ')[1] : '';

devnook added 4 commits May 31, 2024 09:29

Update description logic to be more readable

cba3b17

Adapt to browserImplementation field values

fbd7098

revert dist path

28b2140

lint

095aa9c

devnook mentioned this pull request May 31, 2024

Code Feedback #3

Open

devnook requested a review from jcscottiii May 31, 2024 11:26

jcscottiii reviewed May 31, 2024

View reviewed changes

jcscottiii mentioned this pull request May 31, 2024

Add typescript support and openapi integration with backend #5

Open

Update baseline description and rename weidely/limited to available/u…

73d57ae

…navailable

jcscottiii approved these changes Jun 4, 2024

View reviewed changes

devnook added 2 commits June 5, 2024 08:48

check fo baselineDate

1a8cba0

Merge branch 'init-branch' into code-review/3

c9b2a5b

devnook merged commit ca219cc into init-branch Jun 5, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code updates according to https://github.com/GoogleChrome/baseline-status/issues/3 #4

Code updates according to https://github.com/GoogleChrome/baseline-status/issues/3 #4

devnook commented May 31, 2024

tomayac commented May 31, 2024

devnook commented May 31, 2024

tomayac commented May 31, 2024

devnook commented May 31, 2024

jcscottiii left a comment

jcscottiii May 31, 2024

devnook Jun 4, 2024

jcscottiii May 31, 2024

devnook Jun 4, 2024

jcscottiii May 31, 2024

devnook Jun 4, 2024

devnook commented Jun 4, 2024

devnook commented Jun 4, 2024

jcscottiii left a comment

jcscottiii Jun 4, 2024

devnook Jun 5, 2024

	const year = baseline === 'newly' ? baselineDate.split(' ')[1] : '';
	const year = baseline === 'newly' && baselineDate ? baselineDate.split(' ')[1] : '';

Code updates according to https://github.com/GoogleChrome/baseline-status/issues/3 #4

Code updates according to https://github.com/GoogleChrome/baseline-status/issues/3 #4

Conversation

devnook commented May 31, 2024

tomayac commented May 31, 2024

devnook commented May 31, 2024

tomayac commented May 31, 2024

devnook commented May 31, 2024

jcscottiii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devnook commented Jun 4, 2024

devnook commented Jun 4, 2024

jcscottiii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment