Skip to content
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

Fixed snackbar console error #38365

Closed
wants to merge 2 commits into from

Conversation

manjeet-wisetr
Copy link

Description

Console error when click on snackbar:
react-dom.js?ver=17.0.1:4088 Uncaught TypeError: Cannot read properties of undefined (reading 'current') at dismissMe (components.js)

Screenshots

snackbar-console-error

Types of changes

check added on listRef object in dismissMe function in snackbar component

Console Error : 
react-dom.js?ver=17.0.1:4088 Uncaught TypeError: Cannot read properties of undefined (reading 'current') at dismissMe (components.js)
@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Jan 31, 2022
@github-actions
Copy link

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @manjeet-wisetr! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@ciampo ciampo requested review from mirka and ciampo February 1, 2022 13:54
@ciampo ciampo added [Feature] Component System WordPress component system [Package] Components /packages/components [Type] Bug An existing feature does not function as intended labels Feb 1, 2022
Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

Thanks for contributing! The fix looks sensible to me 👍

Before merging, could we:

  • fix the linting errors
  • add a changelog entry for this bug fix

I think the e2e test failure is a flake... let's see if it goes away on the next run.

@alexstine
Copy link
Contributor

@mirka Just a quick question, is this a side-effect of a bigger problem? Why would this code error out? It's a simple focus. Maybe changing when useEffect fires would be better?

@mirka
Copy link
Member

mirka commented Feb 1, 2022

is this a side-effect of a bigger problem?

My read of the problem was that the listRef focus call was added under the assumption that a Snackbar would always be rendered as a child of SnackbarList, which isn't true. So the listRef prop is simply undefined in that case. Do you see deeper problems here? I don't have historical context around this component, but nothing stands out to me in particular (#34736). Suggestions welcome!

@alexstine
Copy link
Contributor

@mirka I don't have historical context around here either. Just wanted to ask to be sure. Focus relates to accessibility and I've got my eye on accessibility going forward to try to prevent regressions.

@mirka
Copy link
Member

mirka commented Feb 1, 2022

@alexstine Ah ok thanks, I see your point. This PR does not introduce a regression in that sense, but it doesn't solve the focus restoration problem either, so I made a new issue for it (#38425).

Comment on lines 68 to 71
if ( listRef && listRef.current ) {
listRef.current.focus();
}

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if ( listRef && listRef.current ) {
listRef.current.focus();
}
listRef.current?.focus();

suggestion (non-blocking): We could use "Optional chaining" here.

Copy link
Author

Choose a reason for hiding this comment

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

yes but apply optional chaining from listRef
image

Copy link
Member

Choose a reason for hiding this comment

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

Thank works as well.

Only the changelog entry is missing, and we can merge this PR.

Copy link
Author

Choose a reason for hiding this comment

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

changelog entry added

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Thank you. Since the changelog is related to this change, I think it should be part of this PR.

Sorry, I probably should've mentioned this initially.

manjeet-wisetr added a commit to manjeet-wisetr/gutenberg that referenced this pull request Mar 23, 2022
manjeet-wisetr added a commit to manjeet-wisetr/gutenberg that referenced this pull request Mar 23, 2022
Copy link
Member

@Mamaduka Mamaduka left a comment

Choose a reason for hiding this comment

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

Hi, @manjeet-wisetr

Can you consolidate the changelog commit with this PR?

@t-hamano
Copy link
Contributor

t-hamano commented Dec 8, 2023

Hi @manjeet-wisetr,

The Snackbar component has been migrated to Typescript by #45472. At the same time, it appears that changes similar to this PR were made. Therefore, I would like to close this PR.

Thank you for your contribution!

@t-hamano t-hamano closed this Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Component System WordPress component system First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
Status: Abandoned ⛔️
Development

Successfully merging this pull request may close these issues.

6 participants