[web] Agama core/Section improvements #816
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dgdavid
force-pushed
the
section-no-title
branch
from
99c28c5
to
c459324
Compare
dgdavid
force-pushed
the
section-no-title
branch
2 times, most recently
from
aad3182
to
7379ff3
Compare
dgdavid
force-pushed
the
section-no-title
branch
4 times, most recently
from
7094689
to
a61df81
Compare
dgdavid
commented
Oct 31, 2023
| @@ -149,10 +147,19 @@ const icons = { | |||
| * | |||
| */ | |||
| export default function Icon({ name, className = "", size = 32, ...otherProps }) { | |||
| if (!icons[name]) { | |||
| console.error(sprintf(_("Icon %s not found!"), name)); | |||
There was a problem hiding this comment.
NOTE: I wonder if worth it adding a process.env.NODE_ENV check for not delivering these console.error in the production build.
Suggested change
| console.error(sprintf(_("Icon %s not found!"), name)); | |
| process.env.NODE_ENV !== "production" && console.error(sprintf(_("Icon %s not found!"), name)); |
or even to have a logError util for using it in every place we do a console.error
const logError = (error) => {
if (process.node.NODE_ENV !== "production") {
console.error(error);
}
};
Suggested change
| console.error(sprintf(_("Icon %s not found!"), name)); | |
| logError(sprintf(_("Icon %s not found!"), name)); |
Not sure if webpack automatically drop these blocks from the production build. Has to check.
There was a problem hiding this comment.
I've checked by my own that blocks conditioned to process.env.NODE_ENV !== 'production' are not shipped in the production build. Which is great. So, creating an issue for wrap all console.errors
|
Oh, it looks like you have a conflict to fix (originated after merging #831). |
By using aria-label and aria-laballedby attributes.
And set the aria-busy attribute when it's in loading mode.
Trust in the truthy value of props instead of checking if present and not empty. See https://developer.mozilla.org/en-US/docs/Glossary/Truthy
Beacuse crypto.randomUUID() was added in version 22.1.0 [1][2] but the jest-environment-jsdom dependency will be updated with jest v30[3] [1] jsdom/jsdom#3551 [2] https://github.com/jsdom/jsdom/blob/master/Changelog.md#2210 [3] jestjs/jest#13825 (comment)
Because it's no longer possible to mock some window properties since jsdom 21.0.0 and it was updated to version 22.1.0 in previous commits. See https://github.com/jsdom/jsdom/blob/master/Changelog.md#2100 and jsdom/jsdom#3492
Output the error to the console instead of rendering it in the UI.
Checking the data-icon-name attribute.
dgdavid
force-pushed
the
section-no-title
branch
from
eedc325
to
7daef3e
Compare
imobachgs
approved these changes
Oct 31, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
We've identified a use case where a section without title makes a lot of sense. The introductory sections at the top of the page, which is the case of current Software page at the time of writing.
And probably more use cases will arise.
Solution
To add support for sections without title and take the opportunity for improving the component a little bit and start adding support for a11n.
Additionally, the layout/Icon component has been improved a bit too, for outputting errors to the console instead of the UI and for including the icon name as data attribute of the rendered SVG for easily identify it.
Side effects
As part of this PR
jsdomdependency has been updated by using theoverridespackage.json key because of the use ofcrypto.randomUUIDand a few tests using mocks forwindown.locationhas to be rewritten since it's no longer possible to mock it anymore. See https://github.com/jsdom/jsdom/blob/master/Changelog.md#2100 and jsdom/jsdom#3492 to know more.In any case,
jest-environtment-jsdomis about to update it in the next version, jestjs/jest#13825 (comment). So, we're prepared already.Testing