-
Notifications
You must be signed in to change notification settings - Fork 81
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(NcAppSettingsDialog): unregisterSection
should remove the section instead of remove all other
#4798
Conversation
I guess this fixes nextcloud/spreed#10858 ? |
Not sure about this. Duplicate IDs are a HTML validation error and no one reads warnings :( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
selectedSection
and sorting don't work on dynamic sections rendering
tests/unit/components/NcAppSettingsDialog/register-unregister.spec.ts
Outdated
Show resolved
Hide resolved
tests/unit/components/NcAppSettingsDialog/register-unregister.spec.ts
Outdated
Show resolved
Hide resolved
This error is caused by a typo in a provide() {
return {
registerSection: this.registerSection,
unregisterSection: this.registerSection,
}
}, |
491acbc
to
9f0c2f8
Compare
Yes that is part of this fixes :) |
@nickvergessen duplicated IDs still throw an error, only the duplicated name is a warning now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine by issue solving.
But I'm not a fun of testing implementation in unit tests, because it doesn't help to check the unit and prevent further refactoring without rewriting the unit test. I can add an alternative test if it is ok for you.
tests/unit/components/NcAppSettingsDialog/register-unregister.spec.ts
Outdated
Show resolved
Hide resolved
tests/unit/components/NcAppSettingsDialog/register-unregister.spec.ts
Outdated
Show resolved
Hide resolved
…on instead of remove all other Also fix some minor issues and only warn on duplicated name instead of error Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
…ntation Co-authored-by: Grigorii K. Shartsev <me@shgk.me> Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
3503b9a
to
06b5c2b
Compare
☑️ Resolves
There was a logic error where unregister removed all but the unregistered section.
Also fix some minor issues and only warn on duplicated name instead of error.
🏁 Checklist