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.

Sign up for GitHub

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

Jump to bottom

feat : add usePointerEvent props #4553

Merged
merged 2 commits into from
Mar 4, 2024
Merged

feat : add usePointerEvent props #4553

merged 2 commits into from
Mar 4, 2024

Conversation

yuki0410-dev
Copy link
Contributor

I want to fix a bug I reported in a previous PR,
Inspired by the comments in the previous PR, we have modified the policy to allow users to freely choose by Props.

pullrequest[bot]
pullrequest bot reviewed Mar 1, 2024
View reviewed changes
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.

@yuki0410-dev you can click here to see the review status or cancel the code review job.

pullrequest[bot]
pullrequest bot reviewed Mar 1, 2024
View reviewed changes
Copy link

@pullrequest pullrequest bot left a comment •
edited
Loading

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

+ 178
- 55

85% JavaScript (tests)
14% JavaScript
1% Markdown

Type of change

Feature - These changes are adding a new feature or improvement to existing code.

@codecov Codecov
Copy link

codecov bot commented Mar 1, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.65%. Comparing base (6852294) to head (046c42d).

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4553      +/-   ##
==========================================
+ Coverage   95.52%   95.65%   +0.13%     
==========================================
  Files          29       29              
  Lines        2569     2579      +10     
  Branches     1055     1069      +14     
==========================================
+ Hits         2454     2467      +13     
+ Misses        115      112       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yuki0410-dev
yuki0410-dev commented Mar 1, 2024
View reviewed changes
test/week_test.test.js Outdated
let dayMouseEntered = null;

function onDayMouseEnter(day) {
dayMouseEntered = day;
}

const weekStart = utils.newDate();
const week = shallow(
const week = mount(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shallow was changed to mount because simulate("pointerenter") could not be fired correctly

Copy link

Choose a reason for hiding this comment

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

Thanks, that's great to know. It might actually be worth adding that as a comment inline above this line.

Image of Ryan Ryan

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 have added a comment.

Copy link

Choose a reason for hiding this comment

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

That looks great and makes a big difference. Thank you!

Image of Ryan Ryan

pullrequest[bot]
pullrequest bot reviewed Mar 1, 2024
View reviewed changes
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 is mostly adding a flag for if mouse is available and adding tests accordingly. Looks good!

Image of Simon Simon

Reviewed with ❤️ by PullRequest

pullrequest[bot]
pullrequest bot reviewed Mar 1, 2024
View reviewed changes
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 is looking really good. Thanks for taking the feedback and submitting this new PR. It's also very appreciated that you took the time to add tests. I just had a few minor comments below.

Nice work!

Image of Ryan Ryan

Reviewed with ❤️ by PullRequest

docs/index.md
@@ -160,3 +160,4 @@
| `wrapperClassName` | `string` | | |
| `yearDropdownItemNumber` | `number` | | |
| `yearItemNumber` | `number` | `DEFAULT_YEAR_ITEM_NUMBER` | |
| `usePointerEvent` | `bool` | `false` | |
Copy link

Choose a reason for hiding this comment

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

It could be useful to add a brief note about what usePointerEvent does and when to use it.

🔹 Improve Readability (Nice to have)

Image of Ryan Ryan

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added description to docs/datepicker.md, since the description of other Props was also written there.

Copy link

Choose a reason for hiding this comment

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

Yep, that's my mistake, I meant to make that comment on that file instead. Thanks!

Image of Ryan Ryan

test/day_test.test.js Outdated
@@ -932,6 +932,27 @@ describe("Day", () => {
});
});

describe("pointer enter", () => {
var onMouseEnterCalled;
Copy link

Choose a reason for hiding this comment

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

As more tests are added in this describe block over time, it feels like this could get a bit harder to maintain. Is there any reason to not just define these per-test as needed?

🔹 Best Practices (Nice to have)

Image of Ryan Ryan

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 had added a test for Pointer Event in parallel with mouse Event as well as others,
I removed the describe section and added an it section, is this correct?

Copy link

Choose a reason for hiding this comment

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

Yes I think that looks good.

Image of Ryan Ryan

@yuki0410-dev
Copy link
Contributor Author

Thank you for your review.
We have made corrections in response to your comments.

@yuki0410-dev yuki0410-dev mentioned this pull request Mar 2, 2024
Closed
@yuki0410-dev
Copy link
Contributor Author

I have closed my previous PR.

Is there anything else I can do to get this PR merged?

pullrequest[bot]
pullrequest bot reviewed Mar 2, 2024
View reviewed changes
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.

I have closed my previous PR.

Is there anything else I can do to get this PR merged?

Thanks for all of your hard work and adjustments to the feedback @yuki0410-dev. We just have to wait for one of the maintainers to approve and merge this.

Image of Ryan Ryan

martijnrusschen
martijnrusschen reviewed Mar 3, 2024
View reviewed changes
Copy link
Member

@martijnrusschen martijnrusschen left a comment

Choose a reason for hiding this comment

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

These changes look good, however the use of Enzyme is deprecated and I'd rather not introduce more test using Enzyme. Can you switch the test cases to React Testing Library?

@yuki0410-dev
Copy link
Contributor Author

@martijnrusschen
Thanks for your advice.
I have tried to replace enzyme to RTL as best I can, what do you think?

pullrequest[bot]
pullrequest bot reviewed Mar 4, 2024
View reviewed changes
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 #4553 up until the latest commit (046c42d). No further issues were found.

Reviewed by:

Image of Ryan Ryan

@martijnrusschen martijnrusschen merged commit 42d88b8 into Hacker0x01:main Mar 4, 2024
6 checks passed
@yuki0410-dev yuki0410-dev deleted the feat/usePointerEvent branch March 5, 2024 06:36
@pullrequest pullrequest bot mentioned this pull request Mar 5, 2024
Closed
@yuki0410-dev yuki0410-dev mentioned this pull request Mar 10, 2024
Merged
@pullrequest pullrequest bot mentioned this pull request Mar 14, 2024
Closed
3 tasks
@IrvingYHM IrvingYHM mentioned this pull request Jun 2, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pullrequest pullrequest[bot] pullrequest[bot] left review comments

@martijnrusschen martijnrusschen Awaiting requested review from martijnrusschen

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yuki0410-dev @martijnrusschen