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 app settings dialog #3025

Closed
wants to merge 2 commits into from

Conversation

nickvergessen
Copy link
Contributor

Before After
Peek 2022-08-12 13-31 Peek 2022-08-12 14-36

Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen added bug Something isn't working 3. to review Waiting for reviews labels Aug 12, 2022
@nickvergessen nickvergessen added this to the 6.0.0 milestone Aug 12, 2022
@nickvergessen nickvergessen requested review from PVince81, vinicius73, skjnldsv, marcoambrosini, a team and raimund-schluessler and removed request for a team August 12, 2022 12:37
@nickvergessen nickvergessen added the regression Regression of a previous working feature label Aug 12, 2022
@raimund-schluessler
Copy link
Contributor

Just for reference, the respective lines were added in #2719.

Copy link
Contributor

@raimund-schluessler raimund-schluessler left a comment

Choose a reason for hiding this comment

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

With these changes, scrolling the components content in the docs does not work anymore. Maybe one rather needs to adjust the usage in Talk?

@nickvergessen
Copy link
Contributor Author

The usage in Talk is AppSettingsDialog with tones of AppSettingsSection inside.

Not sure if should be necessary for an app to overwrite overflows of a stand-alone modal component?
https://github.com/nextcloud/spreed/blob/d01450f87a68454a7acf6fa8e51ad8f0bec40ce6/src/components/ConversationSettings/ConversationSettingsDialog.vue

@raimund-schluessler
Copy link
Contributor

The usage in Talk is AppSettingsDialog with tones of AppSettingsSection inside.

Quite the same happens in the docs example, no?

Not sure if should be necessary for an app to overwrite overflows of a stand-alone modal component? https://github.com/nextcloud/spreed/blob/d01450f87a68454a7acf6fa8e51ad8f0bec40ce6/src/components/ConversationSettings/ConversationSettingsDialog.vue

I guess not. Does it maybe work if you remove the adjustments.

@nickvergessen
Copy link
Contributor Author

The only way I got it working is with these adjustments...

@raimund-schluessler
Copy link
Contributor

Really weird. There must be a reason, it works in the docs, but breaks with your changes.

@raimund-schluessler
Copy link
Contributor

I cloned and build the Talk app and I couldn't reproduce the scrolling issue in the settings dialog with the Talk master branch. It works just fine (besides #3029).

But I have the assumption that it could depend on how the bundler packed everything together, since this rule
https://github.com/nextcloud/nextcloud-vue/blob/0324a18e33e390e27549cf2c405ed70df2d41161/src/components/AppSettingsDialog/AppSettingsDialog.vue#L362-L365
conflicts with this rule
https://github.com/nextcloud/nextcloud-vue/blob/0324a18e33e390e27549cf2c405ed70df2d41161/src/components/Modal/Modal.vue#L868-L869
and the order of them in the bundle (which might vary) determines which one applies.

So the rule overwriting the style for AppSettingsDialog needs to be more specific. I will create a PR over the weekend.

@nickvergessen
Copy link
Contributor Author

Thanks for diving in as so often recently @raimund-schluessler

@raimund-schluessler
Copy link
Contributor

Alternative solution is in #3035.

@raimund-schluessler raimund-schluessler added 2. developing Work in progress feature: modal Related to the modal component and removed 3. to review Waiting for reviews labels Aug 15, 2022
@raimund-schluessler
Copy link
Contributor

Closed with #3035 and nextcloud/spreed#7745.

@raimund-schluessler raimund-schluessler deleted the bugfix/noid/fix-app-settings-dialog branch August 16, 2022 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress bug Something isn't working feature: modal Related to the modal component regression Regression of a previous working feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants