Fix VO issue for date ranges #1555
Conversation
monokrome
changed the title
monokrome
force-pushed
the
bailey-bugfix-vo-range-selection
branch
from
521e02b
to
0170b81
Compare
We may want different phrases for spans vs single selections. Need to look further into this!
|
@ljharb Apologies for confusion - this isn't ready for review yet. Does creating a new PR cause it to be considered ready for review? Sorry if you got pinged about this or anything, as I know it's late. |
monokrome
added
the
semver-patch: fixes/refactors/etc
|
@monokrome there's a new "draft" status you could have used; but either way no worries! ping me when it's ready for review. |
|
@ljharb How do you enable the draft status? Is it a label, a new feature in GitHub, or something else? |
|
@monokrome it's (recently) in a dropdown on the "create pull request" button; non-draft PRs can't be made drafts. |
|
In a more old-school way, you can also just clarify in the PR description and add a WIP tag to the PR! :) |
|
@ljharb I added a test (was going to do this anyway) last night, but not 100% sure what other tests I should add. Specifically, I think that we should have tests to ensure the styles are applied appropriately to |
|
Can you add a bit of context to the description as to what this is fixing? QQ: are you intending to follow up on adding a phrase for within range in this PR or in a follow-up (I don't want to merge in early if so)? |
|
@majapw Added in description! Thanks 😺 |
|
@ljharb I'm confused. I'm merged because it said you approved, but I just noticed that it says you requested a review from maja. Do we expect more than one approving review to merge? I can revert if so. (maja's only comment was regarding the description and it wasn't in a review, so I'm not sure that she wants to review or not) |
|
If it's stamped, it's stamped! def fine to merge. |
|
@monokrome we don't have any such policy here, was more thinking that it's always better to get more eyes :-) if it was a blocker tho, i'd not have stamped. |
There was a problem hiding this comment.
@monokrome I found an issue in your code that will likely cause some regressions. Please check
| ariaLabel = getPhrase(dateIsUnavailable, formattedDate); | ||
| if (modifiers.has('selected-start') && dateIsSelectedAsStartDate) { | ||
| return getPhrase(dateIsSelectedAsStartDate, formattedDate); | ||
| } if (modifiers.has('selected-end') && dateIsSelectedAsEndDate) { |
There was a problem hiding this comment.
I think you're missing the else part of this logic
There was a problem hiding this comment.
oh wait I just noticed that you're returning so this ends up being technically ok, but visually confusing (because it looks like an else if but it's actually a standalone if statement
There was a problem hiding this comment.
whoops, that'd be great to clean up in a followup :-) @monokrome
There was a problem hiding this comment.
@backwardok @ljharb hmm... Think I should use an else?
Feels bad to use let when we don't need it, but also feels worse to have a final return in a branch instead of in the function's root block. Feels like it could lead to confusing behavior?
I can change it if that's our code style, though!
There was a problem hiding this comment.
It shouldn't use an else; but a newline should be inserted between } and if.
This fixes an issue where VoiceOver and other accessibility tools don't let people know that dates are selected when they are somewhere between the start and end date of a date range/span.