Skip to content

Analytics: Fix custom date range calendar styling in WP 6.1 #35649

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
Merged
octaedro merged 4 commits into from
Nov 25, 2022
Merged

Analytics: Fix custom date range calendar styling in WP 6.1 #35649

octaedro merged 4 commits into from
Nov 25, 2022

Conversation

@mattsherman
Copy link
Contributor

@mattsherman mattsherman commented Nov 20, 2022

All Submissions:

Changes proposed in this Pull Request:

This PR includes the react-dates CSS that was previously included via WordPress. It is no longer included by WordPress 6.1+.

This CSS is required to properly style the calendar in the Analytics custom date range dropdown.

Screenshot 2022-11-20 at 10 12 15

Closes #35645.

  • This PR is a very minor change/addition and does not require testing instructions (if checked you can ignore/remove the next section).

How to test the changes in this Pull Request:

  1. Go to any Analytics page.
  2. Click on the "Date range" and click "Custom".
  3. Verify that the calendar styling is fixed (see above).

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you created a changelog file for each project being changed, ie pnpm --filter=<project> changelog add?

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

@mattsherman mattsherman added Bug The issue is a confirmed bug. package: @woocommerce/components issues related to @woocommerce/components Reports/Analytics Issues related to analytics section. focus: components labels Nov 20, 2022
@mattsherman mattsherman self-assigned this Nov 20, 2022
@mattsherman mattsherman requested a review from a team November 20, 2022 15:44
@github-actions
Copy link
Contributor

github-actions bot commented Nov 20, 2022 •
edited
Loading

Test Results Summary

Commit SHA: f339531

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 50s
E2E Tests186006019215m 36s
To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

@mattsherman mattsherman marked this pull request as ready for review November 20, 2022 21:50
@samueljseay samueljseay added this to the 7.2.0 milestone Nov 21, 2022
louwie17
louwie17 previously requested changes Nov 22, 2022
View reviewed changes
Copy link
Contributor

@louwie17 louwie17 left a comment

Choose a reason for hiding this comment

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

Just left a suggested change to import it from the installed package instead.

packages/js/components/src/calendar/style.scss Outdated
@@ -1,3 +1,5 @@
@import "./react-dates.scss";
Copy link
Contributor

@louwie17 louwie17 Nov 22, 2022

Choose a reason for hiding this comment

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

how come you copy/pasted the entire css file of react-dates.scss in our repo, we should be able to import it from the installed package.
I tried it locally, it does look like our css-loader is a bit broken, so you did have to import it like this:
@import '../node_modules/react-dates/lib/css/_datepicker.css';

Copy link
Contributor Author

@mattsherman mattsherman Nov 22, 2022

Choose a reason for hiding this comment

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

Honestly, I copied the approach @wordpress/components had used. It never dawned on me to just import from the installed package. I will look into that. Though, I do wonder if there are any differences... perhaps even "just" the RTL handling.

Copy link
Contributor Author

@mattsherman mattsherman Nov 22, 2022

Choose a reason for hiding this comment

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

Addressed this in 930cb5c

@mattsherman mattsherman requested a review from a team November 22, 2022 19:56
@mattsherman
Copy link
Contributor Author

mattsherman commented Nov 22, 2022

Ready for a re-review!

@joelclimbsthings joelclimbsthings self-requested a review November 22, 2022 21:03
joelclimbsthings
joelclimbsthings previously approved these changes Nov 22, 2022
View reviewed changes
Copy link
Contributor

@joelclimbsthings joelclimbsthings 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.

Thanks for the fix @mattsherman . Tested in Firefox/Chrome and compared to trunk; BIG improvement! Interesting work-around with the imports as well. 🚢 :shipit:

Edit: I also took the liberty of reruning the e2e tests which had failed. Looked like an unrelated timeout.

@mattsherman mattsherman dismissed louwie17’s stale review November 22, 2022 21:47

Address issue and Joel re-reviewed

@octaedro octaedro force-pushed the fix/analytics-daterange-custom-wp61 branch from 930cb5c to f339531 Compare November 24, 2022 14:23
@octaedro
Copy link
Contributor

octaedro commented Nov 24, 2022

Hey @joelclimbsthings,

I just rebased and pushed this PR. Could you take another look at this PR?

octaedro
octaedro approved these changes Nov 25, 2022
View reviewed changes
Copy link
Contributor

@octaedro octaedro left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@octaedro octaedro merged commit ffb256d into trunk Nov 25, 2022
@octaedro octaedro deleted the fix/analytics-daterange-custom-wp61 branch November 25, 2022 14:57
@github-actions github-actions bot mentioned this pull request Nov 25, 2022
Merged
github-actions bot pushed a commit that referenced this pull request Nov 25, 2022
@mattsherman
Analytics: Fix custom date range calendar styling in WP 6.1 (#35649)
2983b9a 
* Include react-dates styles (no longer included by WP 6.1+)

* Remove padding from filters popover content

* Changelog

* Include react-dates CSS directly from module instead of copy/pasting.
samueljseay pushed a commit that referenced this pull request Nov 28, 2022
@github-actions @mattsherman
Cherry pick 35649 into release/7.2 (#35729)
3758f81 
Analytics: Fix custom date range calendar styling in WP 6.1 (#35649)

* Include react-dates styles (no longer included by WP 6.1+)
* Remove padding from filters popover content
* Include react-dates CSS directly from module instead of copy/pasting.

Co-authored-by: Matt Sherman <[email protected]>
@nathanss nathanss mentioned this pull request Nov 29, 2022
Closed
5 tasks
@kaushikasomaiya kaushikasomaiya mentioned this pull request Dec 3, 2022
Closed
5 tasks
@kaushikasomaiya
Copy link
Contributor

kaushikasomaiya commented Dec 9, 2022 •
edited
Loading

@mattsherman Wondering if this temporary fix needs to be removed since we've imported react dates, on wccom repo: https://github.com/Automattic/woocommerce.com/pull/15003/

@t-hamano t-hamano mentioned this pull request Apr 17, 2023
Closed
@eason9487 eason9487 mentioned this pull request Aug 14, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@louwie17 louwie17 louwie17 left review comments

@joelclimbsthings joelclimbsthings Awaiting requested review from joelclimbsthings

+1 more reviewer

@octaedro octaedro octaedro approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mattsherman mattsherman

Labels

Bug The issue is a confirmed bug. package: @woocommerce/components issues related to @woocommerce/components Reports/Analytics Issues related to analytics section.

Projects

None yet

Milestone

7.2.0

Development

Successfully merging this pull request may close these issues.

Analytics: Custom date range calendar styling is broken in WordPress 6.1

7 participants

@mattsherman @octaedro @kaushikasomaiya @joelclimbsthings @louwie17 @samueljseay