Skip to content

Conversation

@Antreesy
Copy link
Contributor

☑️ Resolves

  • Fix box-sizing property for nested components:
    • If NcModal is mounted inside of #content-vue element, as all other components, it inherits style from NcContent (see here
      33f4af3)
    • But by default, NcModal is mounted in body element, where styles are not overwritten, and all element apart from <table>, <select>, <button> and <input> elements have `box-sizing: content-box by default
    • This will affect <div class="modal-container">, <div class="modal-container__content">, and all that goes to <slot />

🖼️ Screenshots

🏚️ Before 🏡 After
image image

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable

@Antreesy Antreesy added bug Something isn't working 3. to review Waiting for reviews feature: modal Related to the modal component labels Dec 14, 2023
@Antreesy Antreesy added this to the 8.4.0 milestone Dec 14, 2023
@Antreesy Antreesy self-assigned this Dec 14, 2023
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

I wonder if this counts as a breaking change.

Probably may break some modals that had content with styles for default border-box...

But I'd make this change, having different styles depending on mount point isn't good.

And +1 thing to think about global box-sizing: border-box; (#4956 👀)

@susnux
Copy link
Contributor

susnux commented Dec 14, 2023

Probably may break some modals that had content with styles for default border-box...

I think every more complex modal will be broken as most of them are mounted to the body. So this needs to be considered.

@Antreesy
Copy link
Contributor Author

Maybe we put in on the next branch, as we should prepare a migration guide for it anyway? I'm assuming, that more or less complex modals including it already, so with moving to Vue 3 it wouldn't be a huge task to check styles

skjnldsv
skjnldsv previously approved these changes Dec 20, 2023
@ShGKme
Copy link
Contributor

ShGKme commented Dec 20, 2023

@skjnldsv Does your approval mean we may not consider it as a breaking change and merge into v8? 😀

Copy link
Contributor

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

@skjnldsv Does your approval mean we may not consider it as a breaking change and merge into v8? 😀

Nah, should be for v9 then

@skjnldsv skjnldsv dismissed their stale review December 21, 2023 08:42

See comments

@Antreesy Antreesy force-pushed the fix/apply-box-sizing-to-modal-content branch from f1dc89f to 52d444b Compare December 21, 2023 09:03
@Antreesy Antreesy changed the base branch from master to next December 21, 2023 09:03
@Antreesy Antreesy added the vue 3 label Dec 21, 2023
@Antreesy Antreesy requested a review from skjnldsv December 21, 2023 09:04
@Antreesy Antreesy modified the milestones: 8.4.0, 9.0.0 next Vue 3 Dec 22, 2023
@raimund-schluessler raimund-schluessler merged commit 19efde2 into next Dec 22, 2023
@raimund-schluessler raimund-schluessler deleted the fix/apply-box-sizing-to-modal-content branch December 22, 2023 08:44
@susnux susnux mentioned this pull request Jan 23, 2024
1 task
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 bug Something isn't working feature: modal Related to the modal component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants