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

Move close button within modal boundaries #2738

Merged

Conversation

marcoambrosini
Copy link
Contributor

@marcoambrosini marcoambrosini commented Jun 7, 2022

fix #2737

Screen.Recording.2022-06-07.at.16.23.01.mov

Signed-off-by: Marco Ambrosini [email protected]

@marcoambrosini marcoambrosini self-assigned this Jun 7, 2022
@marcoambrosini marcoambrosini added bug Something isn't working enhancement New feature or request 3. to review Waiting for reviews feature: modal Related to the modal component labels Jun 7, 2022
@marcoambrosini marcoambrosini added this to the 5.4.0 milestone Jun 7, 2022
Copy link
Contributor

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Looks good design-wise

@raimund-schluessler
Copy link
Contributor

I didn't test it, but the code looks good. I would just vote for having the default as true and make it a breaking change for merging into master, since we anyway already have some breaking changes in master already that will need a version bump to 6.

@marcoambrosini marcoambrosini changed the title Support close button within modal boundaries Move close button within modal boundaries Jun 8, 2022
@marcoambrosini marcoambrosini added the breaking PR that requires a new major version label Jun 8, 2022
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.

Looks good and works as it should, only https://github.com/nextcloud/nextcloud-vue/pull/2738/files#r892030091 needs to be fixed.

@raimund-schluessler raimund-schluessler modified the milestones: 5.4.0, 6.0.0 Jun 8, 2022
Copy link

@nimishavijay nimishavijay left a comment

Choose a reason for hiding this comment

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

Looks great 🚀

@marcoambrosini marcoambrosini force-pushed the bugfix/noid/put-the-close-button-into-the-modal branch from e87b175 to 6069a1b Compare June 8, 2022 10:46
Signed-off-by: Marco Ambrosini <[email protected]>
@raimund-schluessler raimund-schluessler merged commit 60e81a7 into master Jun 8, 2022
@raimund-schluessler raimund-schluessler deleted the bugfix/noid/put-the-close-button-into-the-modal branch June 8, 2022 11:40
@juliusknorr juliusknorr mentioned this pull request Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews breaking PR that requires a new major version bug Something isn't working enhancement New feature or request feature: modal Related to the modal component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move the close button into the modals
4 participants