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

Timepicker accessibility #476 #520

Merged
merged 13 commits into from
Nov 13, 2024

Conversation

gselderslaghs
Copy link
Member

Proposed changes

Fixes & enhancements based on #476

  • Layout mismatch on mobile spec
  • Layout light theme: color mismatch on input fields in timepicker component
  • Event listeners: Refactored click listeners to focus listener, this is more specific and detects focus by keyboard tabbing
  • Input hours field: fix leading zero's consistency
  • Input hours/minutes: introduced focusout event listener, moved leading zero's to focusout event listener since they are interrupting keyboard input and reduce accessibility
  • Extend twelveHour option in inputFromTextField method
  • Remove autofocus on change since it's interrupting keyboard input and reduce accessibility
  • Make submit/cancel buttons more accessible, introduce focussability and tabbability
  • Fix bug in 24hour user input where hand was not placed correctly on analog clock when changing clock value manually in digital display

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to change).

Checklist:

  • I have read the CONTRIBUTING document.
  • My commit messages follows the conventional commit format
  • [] My change requires a change to the documentation, and updated it accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@Jerit3787
Copy link

Jerit3787 commented Oct 30, 2024

Hi @gselderslaghs, please rebase all PR to branch v2-dev. Thanks

@gselderslaghs gselderslaghs changed the base branch from main to v2-dev October 30, 2024 10:30
@gselderslaghs
Copy link
Member Author

Reviewing my work. Not sure if this PR is fully correct, since I've implemented button classes (L411) and as there are multiple options available, hard-coding the button style would reduce the availability of different button styles when implementing this component.

Since the framework has different button styling available I might need to refactor the codebase in order to apply the button styling variations as an option in the component initializaton. Anyway, not defining a button style might result in accessibility reduction. This also applies to other components in the component library.

  • Should we apply the definition of button styling as a global function instead (eg. based on selected theme)?
  • Should we apply a solution for scenarios where default styling/theme is defined and no button classes are added, so as a result the buttons get focussed anyway?

Copy link
Member

@wuda-io wuda-io left a comment

Choose a reason for hiding this comment

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

LGTM. I think in the near future we should aim to use Temporal for better date time handling.

@wuda-io wuda-io merged commit 196bd69 into materializecss:v2-dev Nov 13, 2024
1 check passed
@wuda-io
Copy link
Member

wuda-io commented Nov 13, 2024

I like the ideas with the Button styling. We should discuss this in a new issue.
I sent you an invitation to join the team. Thank you for the clear PRs.

@gselderslaghs
Copy link
Member Author

LGTM. I think in the near future we should aim to use Temporal for better date time handling.

@wuda-io
I just reviewed the merge commit and noticed there is still an invalidity in CSS styling declaration
This particularly happens within media query match between 601px and 734px (see attached screenshots)
I propose we should use fixed widths for the input fields instead of depending on width percentage within this media query match
Screenshot 2024-11-14 at 14 32 21
Screenshot 2024-11-14 at 14 36 40

I would suggest reopening #476 as I will review and fix the mentioned issue in the next commits

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.

3 participants