Skip to content
This repository was archived by the owner on Oct 19, 2026. It is now read-only.

Display error rather than generate flaw for {{CSSSyntax}} - #4487

Merged
peterbe merged 4 commits into
mdn:mainfrom
teoli2003:make-csssyntax-display-error
Aug 17, 2021
Merged

peterbe merged 4 commits into
mdn:mainfrom
teoli2003:make-csssyntax-display-error

Conversation

@teoli2003

Copy link
Copy Markdown
Contributor

In {{CSSInfo}} (as well as in {{Specification}} and {{Compat}}, although the mechanism is different), when the structured data is missing, an error message is displayed.

In {{CSSSyntax}}, an exception is thrown, the name of the macro is displayed in the page, and a flaw is generated.

The behavior of {{CSSInfo}} is better: there is nothing wrong in the page (as the macro flaw hints) and the message helps diagnose and to fix the actual problem.

This PR fixes this.

cc/ @peterbe: do you think you can still squeeze this review? The only issue in macros preventing me to go down to 0 for MacroExecutionError flaws :-)

@peterbe

peterbe commented Aug 17, 2021

Copy link
Copy Markdown
Contributor

The philosophy for flaws is that they are NOT warnings. They are things you/one should address if time wasn't an issue.
And throwing an error inside a .ejs leads to the effect of it being a flaw. I.e. "Hey! Wake up and correct the content here! ...when time allows" but if you are confident that is not an option, then, correctly, it doesn't need to be a flaw.

Comment thread kumascript/macros/CSSSyntax.ejs Outdated
if (!formattedSyntax) {
throw new Error(`No syntax found in DB for '${name}'`);
formattedSyntax = "<span style=\"color:red;\">" +
`No syntax found in DB for '${name}'` + "</span>";

@peterbe peterbe Aug 17, 2021 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure an inline block of <span> is correct use here? What about a <div class="notecard warning"> instead so there's consistency in alerts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied the style used in CSSInfo, but let's change this. I can even change the one in CSSInfo at the same time to have full consistency.

Let's do this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed (I decided to leave CSSInfo out of this finally).

It looks like this:
Screenshot 2021-08-17 at 18 36 27

@peterbe

peterbe commented Aug 17, 2021

Copy link
Copy Markdown
Contributor

To explain why the tests are failing...
The Kitchensink page must always have 0 flaws.
In your current branch, start yarn dev and go to HTTP://localhost:3000/en-US/docs/MDN/Kitchensink. Does it now have a flaw?

@peterbe peterbe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you merge teoli2003#1 I think this PR will start to pass CI.

@peterbe

peterbe commented Aug 17, 2021

Copy link
Copy Markdown
Contributor

@teoli2003 Perhaps as a follow-up it would be nice if you can change mdn/kitchensink/index.html in mdn/content so that it uses CSSSyntax, also, in a way that doesn't show the warning.

@peterbe
peterbe merged commit d2697f5 into mdn:main Aug 17, 2021
@teoli2003

teoli2003 commented Aug 17, 2021 •

Copy link
Copy Markdown
Contributor Author

@teoli2003 Perhaps as a follow-up it would be nice if you can change mdn/kitchensink/index.html in mdn/content so that it uses CSSSyntax, also, in a way that doesn't show the warning.

@peterbe , in my local dev branch, it doesn't show the warning. I of course can add a case where it does show the warning. Is this what you meant?

Screenshot 2021-08-17 at 20 25 08

@teoli2003

Copy link
Copy Markdown
Contributor Author

@peterbe. Great news! MacroExecutionErrorflaws went down to 4 after this (from around 25 before).

Screenshot 2021-08-19 at 10 41 08

I'll deal with these 4 last ones shortly, once I have some time :-) But another type of flaw is now well under control.

Thanks a lot!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants