-
Notifications
You must be signed in to change notification settings - Fork 144
Conversation
10b4d76
to
be58c1e
Compare
da8050b
to
cedbb96
Compare
PR has been edited👷 This PR has received other commits, so Renovate will stop updating it to avoid conflicts or other problems. If you wish to abandon your changes and have Renovate start over you may click the "rebase" checkbox in the PR body/description. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @justinshreve, I found an issue with the Compare button styles.
Do you think we need to review how we theme and style these buttons across the app?
I think it might be worth a more thorough review of how the PostCSS / colors are injected by Gutenberg, so we can make sure we are doing the right thing. The update here seemed to handle them differently from past versions. It might also be worth a review of our usage to make sure we are using the correct button type in the correct places (primary / pink or default / gray). I think the updated CSS I just pushed should style everything correctly now, and would allow us to unblock the version update. |
13a746b
to
31ab471
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the updated CSS I just pushed should style everything correctly now, and would allow us to unblock the version update.
yes, its looking good to me. Thanks for seeing this one through @justinshreve 🚢
17523e2
to
88decea
Compare
Unfortunately I found another bug that needs fixing when testing parts of the application: When selecting a custom date using the date range filter, selecting a calendar day entry or using the navigation arrows immediately closes the date picker. This does not happen on |
@jeffstieler I didn't quite get this one all the way through last week. Do you mind adopting it and checking out the date picker bug? |
The datepicker issue should be fixed with a2fd4ce. @jeffstieler had narrowed it down to it being an issue between Jeff found this pull which fixed a similar issue happening in Gutenberg core. However, the fix there didn't quite work exactly the same in our case. I'm still not sure why doing the same thing wouldn't work here, but I did eventually realize that we could hook in on the My testing seemed to indicate all is working okay but could use additional eyes and a round of testing to verify. |
@nerrad thanks for that fix! 🙌 I had gotten close, but was definitely missing the In testing it seems that the dropdown modal no longer closes when clicking outside, so I'll try to restore that functionality. |
I think I've got a solution for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is testing well for me, besides the keyboard aspect noted below. Nice work getting to this point.
The keyboard navigation issue deserves some attention and hopefully there is a solution.
<div | ||
className="woocommerce-calendar__react-dates" | ||
ref={ this.nodeRef } | ||
onBlur={ partial( this.keepFocusInside, CONTAINER_DIV ) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keyboard navigation doesn't let you tab past the first arrow 😭 . I'm not sure how to distinguish a blur event via keyboard navigation from blur event via mouse. Here is a suggestion for setting a flag 🤷♀
In any case, until an upstream change happens to react-dates, I think this is an acceptable tradeoff considering the inputs are accessible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keyboard navigation should be fixed with d03971b.
The one questionable bit is the addition of a losesFocusTo
prop to DateRange
. It allows the tabbing to continue past the calendar grid and into the comparison period selector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one Jeff, I never knew about event.relatedTarget!
I agree the added prop losesFocusTo
isn't ideal. In theory, this is temporary code though.
In any case, this is testing well with the keyboard. Just a comment about a leftover console.log.
return; | ||
} | ||
|
||
console.log( 'looks like a mouseUp() blur', e ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a console.log in here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 5e06c5a.
d03971b
to
5e06c5a
Compare
- this fixed tests after an npm install on my end
Add prop for specifying an element that's allowed to take focus from DateRange.
5e06c5a
to
8932e41
Compare
This PR contains the following updates:
2.4.0
->2.6.0
8.0.0
->8.3.1
3.3.0
->3.7.1
1.3.0
->1.5.0
4.5.0
->4.9.1
3.3.0
->3.5.0
2.4.0
->2.8.1
2.3.0
->2.6.0
2.3.0
->2.5.0
3.4.0
->3.6.1
4.1.0
->4.3.0
2.3.0
->2.6.1
1.5.0
->1.8.1
3.2.1
->3.4.0
2.6.0
->2.8.0
2.4.0
->2.8.1
Release Notes
WordPress/gutenberg
v2.6.0
Compare Source
v2.5.0
Compare Source
Renovate configuration
📅 Schedule: "before 3am on wednesday" (UTC).
🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.
♻️ Rebasing: Whenever PR becomes conflicted, or if you modify the PR title to begin with "
rebase!
".👻 Immortal: This PR will be recreated if closed unmerged. Get config help if that's undesired.
This PR has been generated by Renovate Bot. View repository job log here.