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(NcModal): Adjust modal header name and actions #5656

Merged
merged 2 commits into from
Jun 14, 2024
Merged

Conversation

susnux
Copy link
Contributor

@susnux susnux commented May 31, 2024

☑️ Resolves

  • Fix size and margin of header name to work with new server styles on Nextcloud 30+
  • Fix focus-visible, hover and open color of header actions on bright color theme

@artonge this will fix the currently failing visual regression tests of Activity.

🖼️ Screenshots

🏚️ Before 🏡 After
Screenshot_20240531_103246 Screenshot_20240531_103040
Screenshot_20240531_103251 Screenshot_20240531_103009
Screenshot_20240531_103255 Screenshot_20240531_103014

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 3️⃣ Backport to next requested with a Vue 3 upgrade

- Fix size and margin of header name to work with new server styles on Nextcloud 30+
- Fix focus-visible, hover and open color of header actions on bright color theme

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux added bug Something isn't working 3. to review Waiting for reviews labels May 31, 2024
@susnux susnux added this to the 8.12.1 milestone May 31, 2024
@susnux
Copy link
Contributor Author

susnux commented May 31, 2024

/backport to next

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux requested a review from ShGKme May 31, 2024 09:52
@susnux susnux merged commit ea835e4 into master Jun 14, 2024
18 checks passed
@susnux susnux deleted the fix/modal-header branch June 14, 2024 11:42
@Antreesy
Copy link
Contributor

SPDX header is missing 🥲

@@ -214,10 +217,10 @@ export default {
tabindex="-1">
<!-- Header -->
<transition name="fade-visibility" appear>
<div class="modal-header">
<div class="modal-header" data-theme-dark>
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to work if a theme is enforced ⚠️

Copy link
Contributor

Choose a reason for hiding this comment

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

That requires a fix on server I think, to ensure we at least load the dark theme with data attribute if another one is enforced 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then talk has a problem as they are using this to have the dark "chat view" on every theme

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's k, this is an edge case :)
Hope the server fix suits you! 👍

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants