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

Bugfix: Unfocused background page should not be scrollable while modal is open. #87

Merged
merged 3 commits into from
Jun 7, 2023

Conversation

subdavis
Copy link
Contributor

@subdavis subdavis commented May 30, 2023

Bug in oruga overrides causes Modal.vue computed -> scrollClass to always be '' which means that the no scroll behavior gets broken when theme-bulma is installed.


For what it's worth, I actually think the behavior of computedClass(field: string, defaultValue: string, suffix: string = '') in BaseComponentMixin is kinda bad design.

In order to keep from breaking your component each time oruga adds a customizable class, you'll need to update this repo, which seems quite error prone and appears to be what happened here.

// This line serves no purpose and should be removed
const overrideClass = getValueByPath(config, `${this.$options.configField}.${field}.override`, override)

// ... Later in the file, remove the usage of overrideClass and allow the default value to be used even IF `override = true` if there's no explicit override class defined in the config object.
let appliedClasses = /* ... rewrite this ... */

Bug in oruga overrides causes Modal.vue computed -> `scrollClass` to always be `''` which means that the no scroll behavior gets broken when oruga UI is installed.
@jtommy
Copy link
Member

jtommy commented May 30, 2023

Bulma/Buefy adds a class for it, we have to update it

Use is-clipped from bulma
@subdavis
Copy link
Contributor Author

I've updated the PR to use is-clipped as documented in the bulma modal guide: https://bulma.io/documentation/components/modal/

@subdavis
Copy link
Contributor Author

subdavis commented Jun 6, 2023

Is 01df5ec the right change? This is a pretty big usability bug, LMK how I can help get it resolved. Thank you!

@jtommy
Copy link
Member

jtommy commented Jun 7, 2023

@subdavis please, remove noScrollClass. As you can see in buefy example there is no need to apply it

@subdavis
Copy link
Contributor Author

subdavis commented Jun 7, 2023

I don't think that's correct.

This is from the buefy modal docs with a modal open: see is-clipped

Screenshot 2023-06-07 at 8 20 44 AM

This is with a no scroll card: see is-noscroll

Screenshot 2023-06-07 at 8 22 33 AM

I don't know what change you're asking for, but if I remove noScrollClass from the config, no class will be applied at all and this is the whole reason for the current bug.

@jtommy
Copy link
Member

jtommy commented Jun 7, 2023

But you are setting the same class

@subdavis
Copy link
Contributor Author

subdavis commented Jun 7, 2023

I removed noScrollClass

@jtommy jtommy merged commit b4a3ef9 into oruga-ui:master Jun 7, 2023
@subdavis subdavis deleted the patch-1 branch June 7, 2023 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants