-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Calypso UI Components: DateRange: Refactor "header" and "footer" areas of component #94567
Calypso UI Components: DateRange: Refactor "header" and "footer" areas of component #94567
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~241 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~162 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
75db8a2
to
a3adccd
Compare
0e534f3
to
ed62e9e
Compare
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
ed62e9e
to
92ecd23
Compare
92ecd23
to
a661941
Compare
.DayPicker-Day--start.DayPicker-Day--range-start, | ||
.DayPicker-Day--end.DayPicker-Day--range-end { | ||
& .date-picker__day { | ||
.DayPicker-Day--range, |
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.
are these changes intended?
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.
hmmm maybe my rebase got a bit out of control here. Those look like your changes from yesterday with the range styling, right?
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.
Looks like a rebase issue here yes. If you take a look at the highlight colors, is it still working?
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.
Fixing that up to bring all your work back in now. Sorry about that 🙈
TS type error in teamcity:
|
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 refactoring! LGTM 🚀
{ applyText } | ||
</Button> | ||
<div className="date-range__controls"> | ||
{ customTitle ? ( |
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 definitely isn't a blocker, but I'm still thinking maybe we shouldn't hide the help functions when there is a custom title, i.e. it'll be better if the custom title and the helpers could co-exist.
Just something for you to consider. Not a priority right now.
Great work!
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.
Thanks, I agree there may be some way for them to work together in a future iteration. We will figure something out!
Related to https://github.com/Automattic/red-team/issues/178
renderFooter
function + adding this to the docsWhy are these changes being made?
Testing Instructions
/devdocs/design/date-range
default
demo at the top. Check that everything still displays correctly, including when selecting custom dates check that theplease select the first day
text changes toplease select the last day
toreset
as you select date rangescancel
+apply
buttons display below the calendars nowWith custom title
demo it at the topPre-merge Checklist