Skip to content

UIEUS-420: Create monthpicker #555

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

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

elsenhans
Copy link
Contributor

Copy link

github-actions bot commented Jun 3, 2025

Jest Unit Test Results

  1 files  ± 0   39 suites  +1   2m 29s ⏱️ +2s
189 tests + 9  189 ✅ + 9  0 💤 ±0  0 ❌ ±0 
192 runs  +12  192 ✅ +12  0 💤 ±0  0 ❌ ±0 

Results for commit 5cef99a. ± Comparison against base commit 01ef8f6.

♻️ This comment has been updated with latest results.

@elsenhans elsenhans requested a review from a team June 3, 2025 07:15
Copy link
Contributor

@alb3rtino alb3rtino left a comment

Choose a reason for hiding this comment

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

Looking good so far. I left some comments that should be addressed.

Also currently, the dateFormat prop is used directly as the placeholder text in the input field. This means the placeholder is fixed to a specific string pattern (like "YYYY-MM") regardless of the user's locale. For a better user experience, this placeholder could be localized – e.g., "06/2025" for a en-US locale and "06.2025" for a de-DE locale.

Comment on lines 163 to 165
aria-roledescription={calendarRoledescription}
className={css.calendar}
role="application"
Copy link
Contributor

Choose a reason for hiding this comment

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

Im not very familiar with aria roles, but application seems unsuited. When in doubt, it's better to just leave it out.

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/application_role states:

The application role indicates to assistive technologies that an element and all of its children should be treated similar to a desktop application, and no traditional HTML interpretation techniques should be used. This role should only be used to define very dynamic and desktop-like web applications. Most mobile and desktop web apps are not considered applications for this purpose.

Improperly using the application role can unintentionally take away access from information on a web page, so be very mindful of using it. Think hard on if you actually need it and cannot just use a set of other known widgets to accomplish the same task.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was and still am unsure about this.
At first I was using role=dialog, but that led to an issue (asking to use directly <dialog /> instead of just use the role attribute).
Then I just used the datepicker as a guide:
https://github.com/folio-org/stripes-components/blob/8357f35711039f4bbe891811ef0db9637af0e04a/lib/Datepicker/Calendar.js#L527

The keyboard control and the focus are working without the "role" attribute, but the semantic would be missing.
I still think that role=dialog would be the best solution. Maybe the rule, which is causing the issue should be re-configured?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I would like to set it back to role=dialog and accepting or addressing the issue.
@alb3rtino Would that be fine with you?

Copy link
Contributor

Choose a reason for hiding this comment

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

At first I was using role=dialog, but that led to an issue (asking to use directly instead of just use the role attribute).

Seems like an excellent advice. <dialog> is a native HTML element that has role="dialog" built in. Screen readers understand it out of the box and it provides modality, focus management, and Escape key handling. See Accessibility primer for developers.

Also, consider applying grid and gridcell roles to the month grid and the individual month buttons to enhance semantics and accessibility.

Lastly, I recommend testing the component with a screen reader to confirm that all interactive elements are accessible and announced as expected.

Copy link
Contributor Author

@elsenhans elsenhans Jun 4, 2025

Choose a reason for hiding this comment

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

My last try with Orca (for Linux) was like...not really nice. Working somehow but more bad then good.
Instead for VuFind were using NVDA (for Windows) for testing elements.
But I will give it a try if Orca is working better since that time.

Copy link
Contributor Author

@elsenhans elsenhans Jun 4, 2025

Choose a reason for hiding this comment

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

Changed to:

    <div
      aria-label={yearMonthSelection}
      aria-roledescription={calendar' }
      role="dialog"
    >
      <div
        aria-label={yearSelection}
        role="group"
      >  
        Jahresfelder ...
      </div>
      <div
        aria-label={monthSelection}
        role="grid"
      >
        {months.map((month, index) => (
          <div role="row" key={month}>
            <div role="gridcell">
              <Button
                aria-label={ `{month} selected`  || {month} }
                aria-pressed={index === calendarDate?.month}
              >
                {month}
              </Button>
            </div>
          </div>
        </div>

But I still need to test it with Orca

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Das liest Orca nun vor, wenn man via Tastatur über den Monthpicker navigiert:

harvesting start entry required

calender push button
opens menu

calender year and month selection dialog
year selection penal

go to previous year push button
year 2025 spinn button
go to next year push button

month selection

table with 12 rows one column
    
row one column one
jan selected titel button pressed focus mode (if button selected)

row two 
feb titel button not pressed (if button not selected)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now we have those issues, saying we should use the HTML-tag instead of just adding the role attribute:
https://sonarcloud.io/project/issues?id=org.folio%3Aui-erm-usage&pullRequest=555&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true

We could solve 3 of them by using <fieldset> and <table>.
The code would be something like this (and I had to adapt the styling again):

      <fieldset className={css.calendarHeader}>
        <legend className="sr-only">
          {intl.formatMessage({ id: 'ui-erm-usage.monthpicker.yearSelection' })}
        </legend>

        <FormattedMessage id="stripes-components.goToPreviousYear"> ...

        <Col xs={4}>
          <FormattedMessage id="stripes-components.Datepicker.yearControl"> ...
        </Col>

        <FormattedMessage id="stripes-components.goToNextYear"> ...
      </fieldset>
      <div className={css.calendarMonths} aria-label={intl.formatMessage({ id: 'ui-erm-usage.monthpicker.monthSelection' })}>
        <table>
          <tbody>
            {[0, 1, 2].map(row => (
              <tr key={row}>
                {[0, 1, 2, 3].map(col => {
                  const index = row * 4 + col;
                  const month = months[index];
                  return (
                    <td key={month}>
                      <Button
                        aria-label={index === calendarDate?.month ? `${month} selected` : month}
                        aria-pressed={index === calendarDate?.month}
                        buttonStyle={index === calendarDate?.month ? 'primary' : ''}
                        onClick={() => handleMonthSelect(index)}
                      >
                        {month}
                      </Button>
                    </td>
                  );
                })}
              </tr>
            ))}
          </tbody>
        </table>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My suggestion for the eslint issues and missing esc-keyboard function:

  • I am using now <fieldset> instead of <div role="group">.
  • I disabled eslint-rules for <div role="dialog"> and <div role="row">.
  • I am using the HasCommand stripes componente for add to esc-keyboard command

Copy link
Contributor

Choose a reason for hiding this comment

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

Test lacks test cases for different dateFormat values and locales.

@elsenhans elsenhans marked this pull request as draft June 6, 2025 14:06
Copy link

@elsenhans elsenhans marked this pull request as ready for review June 18, 2025 12:51
@elsenhans elsenhans requested a review from alb3rtino June 18, 2025 12:51
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