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

week numbers #53

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

week numbers #53

wants to merge 8 commits into from

Conversation

WickyNilliams
Copy link
Owner

@WickyNilliams WickyNilliams commented Jun 14, 2024

  • adds show-week-numbers option to date/range/multi components
  • adds weeknumber slot to month component
  • adds <col>s plus css parts for each column in month
  • adds weekday part for day headings, weeknumber part for week number heading

Todo

  • test how week numbers are announced in SRs

@raphaelokon
Copy link

@WickyNilliams Hi there. Is there anything we could help with to get the show-week-numbers feature released? Cheers for the great component btw.

@WickyNilliams
Copy link
Owner Author

I just need to merge and release really. Not sure why I didn't at the time, since the feature is complete.

I haven't had much time recently as I've had a lot going on. But I'm planning on putting some hours in later this week. I'll ping you when it's released :)

In the meanwhile, feel free to take a look at the code and give any feedback!

@raphaelokon
Copy link

Thanks @WickyNilliams

I already did! Looks really good, it is a breeze to use Cally.

@raphaelokon
Copy link

@WickyNilliams One thing that might be a tricky one is that if the week-numbers are structurally part of the table, they will be part of the navigation of assistive technologies like NVDA and VoiceOver. Since the week-numbers do not take part in the date selection process this might confuse screenreader users. What do you think?

@WickyNilliams
Copy link
Owner Author

It should announce "30th December, Monday, week 52" (order may not be correct, I'm going from memory), which I think is fine?

The week will only be announced when you change week (move focus vertically). Similar to how day is only announced when you change day (move focus horizontally). So it shouldn't be overly noisy. If you have chosen to show the week number, it feels like you'd want this announced else why show the week number?

@raphaelokon
Copy link

That sounds reasonable. I just wanted to make the point that if the week-numbers are part of the table, the table could be announced with one additional columnwhich might confuse the user. This is just wild guessing tho as I do not have the rendered markup in front of me.

@WickyNilliams
Copy link
Owner Author

Ah I see what you mean. The week number cells are configured as headers, rather than a regular cell. So I hope it should be clear, especially if devs give a reasonable title to the column.

I'll make sure to do some additional screen reader testing before merge

@raphaelokon
Copy link

This is a bit off-topic, but related to weeks → Do you think it would be feasible to let the shortDayName be configurable between 'narrow' | 'short'? → https://github.com/WickyNilliams/cally/blob/main/src/calendar-month/useCalendarMonth.ts#L21

@WickyNilliams
Copy link
Owner Author

following up: I made some tweaks over the weekend to improve how weeks get announced. I need to do more testing with various screen readers to see how they all fare

@WickyNilliams
Copy link
Owner Author

@raphaelokon that might be nice, can you create a new issue for that

@raphaelokon
Copy link

@WickyNilliams#66

@raphaelokon
Copy link

@WickyNilliams Do you need any testing help with the week-numbers? I also created an issue re configuring the weekname abbreviation.

@WickyNilliams WickyNilliams changed the title Feature/week numbers week numbers Nov 3, 2024
@raphaelokon
Copy link

Hi @WickyNilliams
Is there anything we can do ti help out on this one?

@WickyNilliams
Copy link
Owner Author

WickyNilliams commented Feb 18, 2025

The problem I encountered was that the screen reader experience wasn't great. It didn't announce eg "Week 1, January 4th" as I'd hoped. Instead it was "1, January 4th", which is quite confusing.

One option is to offer a weekLabel prop/ week-label attr, so that you provide the labelling necessary in each week number <td>. But I don't particularly like that option, and it doubt it's viable in every language.

Another option would be to hide the column from screen readers, but I don't like that either :(

@raphaelokon
Copy link

I am just thinking about that as I could not find any W3C recommendations how to handle week numbers in calendars. An extra prop sounds overhead to me. Hide informations from screen readers is also not a real solution to the problem and not really inclusive.

I’ll do a local build of that branch (it has conflicts also) and test it. I bet there is a solution.

@WickyNilliams
Copy link
Owner Author

Appreciate it! Happy to have a second set of eyes.

It should be fine to build locally, even with the conflicts. I can resolve them later today though

@raphaelokon
Copy link

I am rethinking … I think regarding localization there must be a way to set the Week label anyways isn’t it?

@WickyNilliams
Copy link
Owner Author

Yes. However, I was hoping to utilize a slotted element, where the slot would have been placed in the <th> for the week column. That way all content is kept in html, and you don't have to deal with whether the label comes before or after the week number

@raphaelokon
Copy link

Okay, that makes sense. I was wondering if a prop would be better tho as you could slot any arbitrary content. Maybe also localization prop that takes also labels for the prev/next buttons.

It would be great to have the conflicts resolved btw. :-)

@WickyNilliams
Copy link
Owner Author

If you just want to test locally the conflicts shouldn't be an issue. Nothing will have affected this feature.

@WickyNilliams
Copy link
Owner Author

In any case, I'll see if I can get them resolved later today

@WickyNilliams
Copy link
Owner Author

@raphaelokon conflicts are now resolved

@raphaelokon
Copy link

@WickyNilliams Cheers and thanks for the resolve.

I was thinking about a more global localization approach to Cally (similar to what lit/localization) does. Because user may want to specify labels for other parts of the Cally UI. I think slotting any possible label that might come could be orthogonal how most teams handle translations in their UI (e.g. translation maps per language). What do you think?

@raphaelokon
Copy link

raphaelokon commented Feb 26, 2025

… having a dedicated prop would effectively resolve the announcement issue isn’t it?

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