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

Array modal's fields now keyboard accessible #4693

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

stuartromanek
Copy link
Member

@stuartromanek stuartromanek commented Aug 19, 2024

Summary

  • Ensures array modals' existing fields are keyboard accessible when modal is opened.
  • Ensures new array items' fields are keyboard accessible when added
  • Change the array editor's isModified method to a computed prop for consistency with other components

What are the specific steps to test this change?

  1. Check these two scenarios
    https://github.com/user-attachments/assets/b1e1f42d-982c-4b13-ba12-4552dae470b3

Tests

https://github.com/apostrophecms/testbed/actions/runs/12794762166

What kind of change does this PR introduce?

(Check at least one)

  • Bug fix

@stuartromanek stuartromanek requested a review from ValJed August 19, 2024 14:42
Comment on lines 28 to 29
const modified = typeof this.isModified === 'function' ? this.isModified() : this.isModified;
if (modified) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@ValJed Unrelated to main pr, but i noticed array's isModified is a function, not a boolean. The bug it was causing was that array modals always acted as if they had been modified and threw a confirm when exiting a clean modal.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh wow nicely spotted, wouldn't it be a better solution to use a computed everywhere or a function ?

Copy link
Contributor

@ValJed ValJed left a comment

Choose a reason for hiding this comment

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

Question about strat.

Comment on lines 28 to 29
const modified = typeof this.isModified === 'function' ? this.isModified() : this.isModified;
if (modified) {
Copy link
Contributor

Choose a reason for hiding this comment

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

oh wow nicely spotted, wouldn't it be a better solution to use a computed everywhere or a function ?

@stuartromanek
Copy link
Member Author

wouldn't it be a better solution to use a computed everywhere or a function ?

That sounds right to me.

I'm already doing too much in some of these PRs .. how would you want to move on a computed? Should i back out my fix and make a ticket for a better a solution? Or leave this and make the ticket anyway, knowing that this could change?

@ValJed
Copy link
Contributor

ValJed commented Jan 13, 2025

@stuartromanek very sorry for that late reply never saw the mention.
I would say you can keep your fix as it is but it would be nice to create a ticket that explain this weird thing (isModified being a computed or a function) and that we should use computed (or function) everywhere for clarity. With a link to the code you added in this PR to check if it's a function or computed, wdyt?

@stuartromanek stuartromanek requested a review from ValJed January 15, 2025 18:18
@stuartromanek
Copy link
Member Author

@ValJed It wasn't as involved to make the upstream change as I thought, so I've converted the array modal's isModified method to a computed property and reverted the double check I initially had here. Running updated tests now but I think this is a solid fix.

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