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

fix(components): [popper] add missing id #9632

Merged
merged 1 commit into from
Sep 13, 2022

Conversation

plehnen
Copy link
Contributor

@plehnen plehnen commented Sep 5, 2022

the currenct popperjs version in element-plus removes the "id"-property. So the aria-describedby reference is pointing to something which doesn't exist in the DOM.
This issue applies for all places where popper is used, i.e. "select", "popover", "date-picker", ...

this fixes #9200 and fixes #8155

@pull-request-triage
Copy link

👋 @plehnen, seems like this is your first time contribution to element-plus.

  • Please make sure that you have read our guidelines and code of conduct before making a contribution.
  • You can comment with /label Components:[component_name] to add a label for which component you are working on.
  • You may join our Discord for staying tuned.

@pull-request-triage pull-request-triage bot added 1st contribution Their very first contribution Needs Review labels Sep 5, 2022
@github-actions
Copy link

github-actions bot commented Sep 5, 2022

@github-actions
Copy link

github-actions bot commented Sep 5, 2022

Hello @plehnen, thank you for contributing to element-plus, please see our guideline to see how to make contribution

@github-actions
Copy link

github-actions bot commented Sep 5, 2022

🧪 Playground Preview: https://element-plus.run/?pr=9632
Please comment the example via this playground if needed.

@plehnen plehnen force-pushed the fix/popper-id branch 4 times, most recently from 9a8eb65 to fa0f498 Compare September 5, 2022 12:11
@github-actions github-actions bot added CommitMessage::Qualified Qualified commit message and removed CommitMessage::Unqualified Unqualified commit message labels Sep 5, 2022
@plehnen plehnen force-pushed the fix/popper-id branch 3 times, most recently from 511e2d9 to 4f37187 Compare September 8, 2022 07:15
@xiaoxian521 xiaoxian521 requested a review from a team September 10, 2022 16:36
@holazz
Copy link
Member

holazz commented Sep 11, 2022

I think a reasonable way to solve this issue is to add the condition that el and preEl are not equal here. Because the two are equal when the triggerTargetEl prop is not passed.

if (isElement(prevEl)) {
;['role', 'aria-label', 'aria-modal', 'id'].forEach((key) => {
prevEl.removeAttribute(key)
})
}

if (isElement(prevEl) && prevEl !== el) {
  // ...
} 

@plehnen plehnen force-pushed the fix/popper-id branch 5 times, most recently from 0716a37 to e410431 Compare September 12, 2022 07:20
@plehnen
Copy link
Contributor Author

plehnen commented Sep 12, 2022

@holazz Thanks! I've reverted the previous changes and added the condition you recommended. Branch is updated and changes are pushed.

@holazz holazz changed the title fix(components): [popover, select] add missing id (#8155) fix(components): [popper] add missing id Sep 12, 2022
@plehnen
Copy link
Contributor Author

plehnen commented Sep 12, 2022

@holazz @tolking do I need to do any further actions to get this merged into dev? Or do I just have to wait now?

@holazz
Copy link
Member

holazz commented Sep 13, 2022

@holazz @tolking do I need to do any further actions to get this merged into dev? Or do I just have to wait now?

Just wait, you don't need to always merge the dev branch into your branch unless your branch conflicts with the dev branch.

@holazz holazz merged commit f7d4354 into element-plus:dev Sep 13, 2022
@element-bot element-bot mentioned this pull request Sep 16, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants