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.

Sign up for GitHub

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

Jump to bottom

Fix additionalTrapElements to accept HTMLelements as well #5120

Merged
merged 1 commit into from
Jan 23, 2024
Merged

Fix additionalTrapElements to accept HTMLelements as well #5120

merged 1 commit into from
Jan 23, 2024

Conversation

GretaD
Copy link
Contributor

@GretaD GretaD commented Jan 23, 2024 •
edited
Loading

☑️ Resolves

🖼️ Screenshots

🏚️ Before 🏡 After
B A

🚧 Tasks

  • Test if it works on mail, it works.

🏁 Checklist

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

@GretaD GretaD added bug Something isn't working 3. to review Waiting for reviews labels Jan 23, 2024
@GretaD GretaD requested a review from susnux January 23, 2024 15:08
@GretaD GretaD self-assigned this Jan 23, 2024
@GretaD GretaD force-pushed the fix/trap-elements-ncdialog branch from 2996abf to 24fe0ac Compare January 23, 2024 15:13
@GretaD GretaD requested a review from ChristophWurst January 23, 2024 15:14
@GretaD
Copy link
Contributor Author

GretaD commented Jan 23, 2024 •
edited
Loading

the error is gone, i can see the tool, but i cannot see the dropdown list. It is shown for a second and then goes away. But im not sure if this issue with the drop down is connected to the trap element

susnux
susnux reviewed Jan 23, 2024
View reviewed changes
src/components/NcDialog/NcDialog.vue
@@ -162,7 +162,14 @@ export default defineComponent({
/** Additional elements to add to the focus trap */
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add that we accept the same as NcModal

susnux
susnux approved these changes Jan 23, 2024
View reviewed changes
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Nice! But will not work. We need to pass it to NcModal.
This was forgotten 🙈 So add additionalTrapElements to modalProps

@GretaD
Copy link
Contributor Author

GretaD commented Jan 23, 2024

Nice! But will not work. We need to pass it to NcModal. This was forgotten 🙈 So add additionalTrapElements to modalProps

ah, ok, will do. test and then will merge this one

@GretaD GretaD force-pushed the fix/trap-elements-ncdialog branch from 24fe0ac to 2729402 Compare January 23, 2024 17:34
@GretaD GretaD enabled auto-merge January 23, 2024 17:34
@GretaD
Copy link
Contributor Author

GretaD commented Jan 23, 2024

tested and works now :)

@GretaD GretaD merged commit 72f56e0 into master Jan 23, 2024
15 checks passed
@GretaD GretaD deleted the fix/trap-elements-ncdialog branch January 23, 2024 17:40
@Pytal Pytal mentioned this pull request Jan 23, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@susnux susnux susnux approved these changes

@ShGKme ShGKme ShGKme approved these changes

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

Assignees

@GretaD GretaD

Labels
3. to review Waiting for reviews bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@GretaD @susnux @ShGKme