feat: Newspack UI styles #2810

laurelfulford · 2023-12-15T02:31:49Z

All Submissions:

Have you followed the Newspack Contributing guideline?
Does your code follow the WordPress' coding standards and VIP Go coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

This PR starts the process of adding more general Newspack UI styles to the Newspack Plugin.

How to test the changes in this Pull Request:

Apply the PR to the epic/ras-acc branch.
Confirm that the styles don't really do anything; they shouldn't be applied to any elements currently in the branch or plugin because they require them to be wrapped in the newspack-ui CSS class.
Confirm that you can load testing components by appending ?ui-demo to the URL, but only when logged in as an administrator.

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 successfully ran tests with your changes locally?

miguelpeixe

Thank you, @laurelfulford! This is looking great!

Some comments inline:

miguelpeixe · 2023-12-18T13:45:39Z

assets/newspack-ui/scss/_modals.scss

+	&__modal {
+		background: var( --newspack-ui-color-body-bg );
+		border-radius: var( --newspack-ui-border-radius );
+		max-width: var( --newspack-ui-modal-width-medium );


I remember we've also established a max-height of 90vh for modals, should we add this here?

Good call! This has been added in 5089ffe.

Thanks! Following the max-height change, shouldn't the overflow change from hidden to auto?

@miguelpeixe I think this should be handled by the overflow added to the .newspack-ui__modal__content class, right? That means the header bit is always visible, though, and only the content bit works -- that's definitely changeable if it'd be better to scroll everything!

(Just let me know if I'm missing something here!)

Currently the .newspack-ui__modal__content is not contained by the max height and not implementing a scrollbar:

This could be fixed through flexboxes, adding display: flex; flex-direction: column; to .newspack-ui__modal.

OHHHH thank you @miguelpeixe! I was testing the styles with the modal checkout, which was adding an inline height and tricking me into thinking the scrolling was working 😅 I see it now.

I went the flexbox route in 210c7ba and works well -- the only bit I'm waffling about is the modal footer. I'm wondering if it should also scroll instead of remaining in place (like it does with the current markup).

I'm thinking this may be something we can test/verify in action and see if it works or not. Based on the mockups, the only case a modal with a footer may need to scroll would be on mobile, and maybe not even then. (I kind of faked the issue in the screenshot below, but the modal with a footer in the demo content isn't actually tall enough to cause this issue):

Hmmmm, I believe the footer should scroll along.

Yeah, agreed -- it seems weird to treat it with the same importance as the header when it's more of a disclaimer space 🙂

I've made an update in 2331596 that makes the header sticky, and moves the overflow so everything else scrolls but the header stays put!

assets/newspack-ui/scss/_modals.scss

miguelpeixe · 2023-12-18T13:50:44Z

assets/newspack-ui/scss/_modals.scss

@@ -0,0 +1,77 @@
+.newspack-ui {
+	&__modal {


I wonder if we should include the functional bits here for handling the visibility state. Doing so would allow us to establish a common animation for them as well. The prototype in Automattic/newspack-blocks#1602 implements the visibility handling with an .open class, which animates the modal.

I think that would be good! It would help avoid duplication for anything else that needs a similar modal, too! 🙂

Cool! We can address that in the next PR so it doesn't block the foundational changes this PR introduces.

Awesome, thanks Miguel! I'll spin up a separate task to keep track of this one.

assets/newspack-ui/scss/_modals.scss

assets/newspack-ui/scss/elements/_index.scss

miguelpeixe · 2023-12-18T14:21:07Z

assets/newspack-ui/scss/variables/_colors.scss

+	--newspack-ui-color-primary-000: #f5fdff;
+	--newspack-ui-color-primary-050: #d6f6ff;
+	--newspack-ui-color-primary-100: #b4e4ff;
+	--newspack-ui-color-primary-200: #93ccfd;
+	--newspack-ui-color-primary-300: #72affb;
+	--newspack-ui-color-primary-400: #528dfc;
+	--newspack-ui-color-primary-500: #36f;
+	--newspack-ui-color-primary-600: #2240d5;
+	--newspack-ui-color-primary-700: #1522af;
+	--newspack-ui-color-primary-800: #0b0b8d;
+	--newspack-ui-color-primary-900: #0d046e;
+	--newspack-ui-color-primary-950: #0e0052;


Not for this PR, but we should tweak this to incorporate a dynamic primary color generated in the backend. The backend logic should use HSL to determine if the publisher's primary color hue is between 50 and 300 (not red). If it's red-ish, it should fall back to a dark grey.

I wonder how to merge that with the proposed grades. If we are to implement different gradings for the publisher's color, I'd propose we do it with opacity rather than dynamically calculating lightness. Still, I'm not a fan of messing with the publisher's brand color in that way...

Correct me if I'm wrong, but I think we only implement the different grades in the checkbox/radio input lists. I think these should always be blue, but we should double-check. If that's the case, we don't need to worry about it.

we should tweak this to incorporate a dynamic primary color generated in the backend

Yes!

I wonder how to merge that with the proposed grades.

I copied these colours over as-is from the _colors.scss file used for the back-end components when I was under the impression a lot more of the back-end would be using Newspack blue. So I think we can probably pare this down to whatever we're actually using.

Correct me if I'm wrong, but I think we only implement the different grades in the checkbox/radio input lists. I think these should always be blue, but we should double-check. If that's the case, we don't need to worry about it.

This is my impression now, too! I'll follow up and confirm just in case 🙂

I ended up removing the blue variables in 3bb8809 -- should I make a separate task for the custom colour stuff?

miguelpeixe · 2023-12-18T14:22:47Z

assets/newspack-ui/scss/variables/_colors.scss

+	--newspack-ui-color-button-bg: var( --newspack-ui-color-gray-900 );
+	--newspack-ui-color-button-bg-hover: var( --newspack-ui-color-gray-800 ); // TODO: Replace w/correct value.
+	--newspack-ui-color-button-text: var( --newspack-ui-color-white );
+	--newspack-ui-color-radio-bg: var( --newspack-ui-color-gray-900 );


Shouldn't this be --newspack-ui-color-primary-500?

Ref:

This stemmed from my confusion around how we're handling the colours! Followed up about that here (p1704831597129219-slack-newspack-reader-activation) for this comment and the previous one -- thanks for pointing these out, I kind of forgot about them! 😅

@miguelpeixe Regarding the above, we'll be going forward with the brand colour for primary actions and black/grey as a fallback -- does it sound okay to update everything to use the black/greys for now?

The above has been done in 3bb8809 kind of as a temporary state 🙂

Edited to add: we also could undo this and have the buttons fall back to the theme defaults for now, but they use the secondary colour, not the primary one. Let me know what you think!

Awesome! Thank you for looking further into this. I like the approach with the temporary state, which might turn out to be the permanent state 🤷

assets/newspack-ui/scss/variables/_spacing.scss

assets/newspack-ui/style.scss

miguelpeixe

Thank you for all the discussions and revisions, @laurelfulford!

laurelfulford · 2024-01-16T17:35:10Z

Yay! Thanks for the great review, @miguelpeixe!

matticbot · 2024-02-06T00:54:35Z

🎉 This PR is included in version 3.0.0-epic-ras-acc.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot · 2024-02-15T08:22:54Z

🎉 This PR is included in version 3.1.0-epic-ras-acc.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot · 2024-02-26T08:50:56Z

🎉 This PR is included in version 3.2.0-epic-ras-acc.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot · 2024-03-25T23:09:45Z

🎉 This PR is included in version 3.6.0-epic-ras-acc.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot · 2024-04-25T13:57:53Z

🎉 This PR is included in version 3.9.0-epic-ras-acc.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot · 2024-05-29T16:25:40Z

🎉 This PR is included in version 4.1.0-epic-ras-acc.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot · 2024-06-05T16:44:41Z

🎉 This PR is included in version 4.2.0-epic-ras-acc.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot · 2024-06-12T16:22:03Z

🎉 This PR is included in version 4.4.0-epic-ras-acc.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

laurelfulford changed the title ~~Feat/newspack UI styles~~ feat: Newspack UI styles Dec 15, 2023

laurelfulford added the [Status] Needs Review The issue or pull request needs to be reviewed label Dec 15, 2023

laurelfulford requested a review from miguelpeixe December 15, 2023 21:17

laurelfulford marked this pull request as ready for review December 15, 2023 21:17

laurelfulford requested a review from a team as a code owner December 15, 2023 21:17

miguelpeixe requested changes Dec 18, 2023

View reviewed changes

dkoo mentioned this pull request Dec 20, 2023

feat: redesign modal checkout Automattic/newspack-blocks#1602

Merged

6 tasks

adekbadek added [Status] Needs changes or feedback The issue or pull request needs action from the original creator and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Jan 5, 2024

miguelpeixe mentioned this pull request Jan 8, 2024

feat: redesign password authentication flow #2826

Merged

6 tasks

laurelfulford added 12 commits January 9, 2024 11:22

feat: start building out Newspack UI styles.

bbd0aa4

feat: correct linting errors

9fc9bc2

feat: correct linting errors

555fde1

fix: rollback unneeded file changes

9b2c94d

feat: add a check for admin for the demo components

447a8e3

fix: correct linting error

a661bbc

fix: corrections for the modal styles

5089ffe

fix: update the styles for now to use greyscale

3bb8809

fix: separate woocommerce, misc styles

d81a167

fix: cleaning up some post-rebase code

616419d

fix: remove unneeded CSS classes for Firefox fallback

4f3dd42

fix: update base spacing variable name

bdb8f38

laurelfulford force-pushed the feat/newspack-ui-styles branch from 706ae78 to bdb8f38 Compare January 10, 2024 00:14

fix: add flexbox to fix modal scrolling

210c7ba

laurelfulford requested a review from miguelpeixe January 13, 2024 00:57

miguelpeixe mentioned this pull request Jan 15, 2024

feat(modal-checkout): redesign variation selection Automattic/newspack-blocks#1643

Merged

6 tasks

fix: make footer scroll with content

2331596

miguelpeixe approved these changes Jan 16, 2024

View reviewed changes

github-actions bot added the [Status] Approved The pull request has been reviewed and is ready to merge label Jan 16, 2024

laurelfulford merged commit 5b05adf into epic/ras-acc Jan 16, 2024
2 checks passed

laurelfulford deleted the feat/newspack-ui-styles branch January 16, 2024 17:46

matticbot added the released on @epic/ras-acc label Feb 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Newspack UI styles #2810

feat: Newspack UI styles #2810

laurelfulford commented Dec 15, 2023 •

edited

Loading

miguelpeixe left a comment

miguelpeixe Dec 18, 2023

laurelfulford Jan 10, 2024

miguelpeixe Jan 10, 2024

laurelfulford Jan 10, 2024

miguelpeixe Jan 11, 2024 •

edited

Loading

laurelfulford Jan 13, 2024

miguelpeixe Jan 15, 2024

laurelfulford Jan 15, 2024

miguelpeixe Dec 18, 2023

laurelfulford Jan 10, 2024

miguelpeixe Jan 10, 2024

laurelfulford Jan 10, 2024

miguelpeixe Dec 18, 2023

laurelfulford Jan 9, 2024

laurelfulford Jan 10, 2024

miguelpeixe Dec 18, 2023

laurelfulford Jan 9, 2024

laurelfulford Jan 9, 2024

laurelfulford Jan 10, 2024 •

edited

Loading

miguelpeixe Jan 10, 2024

miguelpeixe left a comment

laurelfulford commented Jan 16, 2024

matticbot commented Feb 6, 2024

matticbot commented Feb 15, 2024

matticbot commented Feb 26, 2024

matticbot commented Mar 25, 2024

matticbot commented Apr 25, 2024

matticbot commented May 29, 2024

matticbot commented Jun 5, 2024

matticbot commented Jun 12, 2024

feat: Newspack UI styles #2810

feat: Newspack UI styles #2810

Conversation

laurelfulford commented Dec 15, 2023 • edited Loading

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

miguelpeixe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelpeixe Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurelfulford Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelpeixe left a comment

Choose a reason for hiding this comment

laurelfulford commented Jan 16, 2024

matticbot commented Feb 6, 2024

matticbot commented Feb 15, 2024

matticbot commented Feb 26, 2024

matticbot commented Mar 25, 2024

matticbot commented Apr 25, 2024

matticbot commented May 29, 2024

matticbot commented Jun 5, 2024

matticbot commented Jun 12, 2024

laurelfulford commented Dec 15, 2023 •

edited

Loading

miguelpeixe Jan 11, 2024 •

edited

Loading

laurelfulford Jan 10, 2024 •

edited

Loading