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

fix: always show date select #3948

Merged
merged 3 commits into from Jul 11, 2023
Merged

fix: always show date select #3948

merged 3 commits into from Jul 11, 2023

Conversation

kelatev
Copy link
Contributor

@kelatev kelatev commented Feb 24, 2023

image

@gihan9a
Copy link

gihan9a commented Mar 6, 2023

Please merge this ASAP. We are seeing this unexpected selected date on UI

@codecov
Copy link

codecov bot commented Mar 6, 2023

Codecov Report

Merging #3948 (15c1350) into master (8a8cb7d) will decrease coverage by 0.13%.
The diff coverage is 96.50%.

❗ Current head 15c1350 differs from pull request most recent head 7b7b54f. Consider uploading reports for the commit 7b7b54f to get more accurate results

@@            Coverage Diff             @@
##           master    #3948      +/-   ##
==========================================
- Coverage   94.09%   93.96%   -0.13%     
==========================================
  Files          20       20              
  Lines        1726     1890     +164     
  Branches      419      458      +39     
==========================================
+ Hits         1624     1776     +152     
- Misses         31       40       +9     
- Partials       71       74       +3     
Impacted Files Coverage Δ
src/tab_loop.jsx 60.00% <ø> (+2.10%) ⬆️
src/day.jsx 93.43% <60.00%> (-1.27%) ⬇️
src/calendar.jsx 94.09% <93.33%> (-0.10%) ⬇️
src/month.jsx 93.81% <95.50%> (+1.17%) ⬆️
src/date_utils.js 99.21% <100.00%> (+0.08%) ⬆️
src/index.jsx 88.85% <100.00%> (-1.28%) ⬇️
src/week_number.jsx 77.77% <100.00%> (-9.73%) ⬇️
src/year.jsx 97.47% <100.00%> (+0.29%) ⬆️

@martijnrusschen
Copy link
Member

Can you provide more information on the change?

@kelatev
Copy link
Contributor Author

kelatev commented May 25, 2023

  1. I added new prop showDateSelect. When this prop is append (for example: <DatePicker showDateSelect selected={utils.newDate()} />), block .react-datepicker__aria-live is showing "Selected date"
  2. I fixed block .react-datepicker__aria-live when they empty they not showing full (not only hide content)

src/index.jsx Outdated Show resolved Hide resolved
Comment on lines -540 to +543
this.setState({ isRenderAriaLiveMessage: true });
if (this.props.showDateSelect) {
this.setState({ isRenderAriaLiveMessage: true });
}
Copy link
Member

Choose a reason for hiding this comment

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

Why are we adding this behind a prop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this.setState({ isRenderAriaLiveMessage: true }); run in func handleSelect without check for need to renderAriaLiveMessage
Any change date on datepicker run handleSelect and always set isRenderAriaLiveMessage: true

This code fix that. set isRenderAriaLiveMessage: true only if developer set prop showDateSelect

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

✅ This pull request was sent to the PullRequest network.


@kelatev you can click here to see the review status or cancel the code review job.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

PullRequest Breakdown

Reviewable lines of change

+ 9
- 5

71% JavaScript
29% JavaScript (tests)

Type of change

Fix - These changes are likely to be fixing a bug or issue.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

Nice work here. I have just one concern left inline as a reply to another comment.

Image of Graham C Graham C


Reviewed with ❤️ by PullRequest

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

PullRequest reviewed the updates made to #3948 up until the latest commit (7b7b54f). No further issues were found.

Reviewed by:

Image of Graham C Graham C

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

Looks to ensure that the right checks are made related to showing the date selector as described in the PR description. I see no issues to moving this forward

Image of James D James D


Reviewed with ❤️ by PullRequest

@martijnrusschen martijnrusschen merged commit a7cc64f into Hacker0x01:master Jul 11, 2023
4 checks passed
@literalpie
Copy link

literalpie commented Jul 21, 2023

Do I understand right that this change removed aria announcements unless you opt in with the new input? Was this change made based on a misunderstanding of this issue? (the correct solution is to add the CSS styles mentioned in that conversation or import the css styles mentioned in the readme)

If the CSS class is causing too much difficulty, maybe inline styles for the aria live announcement would be a better approach? (I can make a PR for that if people think it makes sense)

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.

None yet

4 participants