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.

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

Fix notices styles in unified sidebar #46819

Merged
merged 3 commits into from
Oct 29, 2020

Conversation

cpapazoglou
Copy link
Contributor

@cpapazoglou cpapazoglou commented Oct 27, 2020

Changes proposed in this Pull Request

Testing instructions

Before After

Text wrapping

Before After
image
  • add ?flags=nav-unification
  • Go to a site with domain warning or edit client/my-sites/domains/components/domain-warnings/index.jsx to force show some

Fixes #46732

@matticbot
Copy link
Contributor

@cpapazoglou cpapazoglou changed the title fix/notices-styles-in-unified-sidebar Fix notices styles in unified sidebar Oct 27, 2020
@cpapazoglou cpapazoglou self-assigned this Oct 27, 2020
@cpapazoglou cpapazoglou added the [Feature] Calypso & wp-admin Navigation All navigation in Calypso and wp-admin, and the unified transitions between the two. label Oct 27, 2020
@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@sixhours
Copy link
Contributor

sixhours commented Oct 27, 2020

Hurrah for no overflow!

I would tweak the padding and spacing a bit for better overall alignment with the other elements in the sidebar.

Before

Screen Shot 2020-10-27 at 4 14 00 PM

After

Screen Shot 2020-10-27 at 4 17 56 PM

I set the height on the notices to 66px and the right-hand padding on the "Renew" CTA to 12px to match the upsell nudge (another possibility is to reduce the right margin/padding on the upsell nudge to 10px from 12px, either way!) Let me know what you think! I know we're working with a tiny amount of space.

@cpapazoglou
Copy link
Contributor Author

cpapazoglou commented Oct 28, 2020

Thanks @sixhours! You are absolutely right, I have wrongly set a static height in the first place. I adjusted both the vertical paddings of the content and padding-right of the action button!

Before After

@frontdevde
Copy link
Contributor

frontdevde commented Oct 28, 2020

Love that we're adjusting the alignment and text wrapping here. Nice change! 👍

I'm slightly concerned the color change might impact the nudge conversion though.

@sfougnier Would love to hear your thoughts on the following:

Reading p2-pbAPfg-103 it appears that nudge conversion is a key concern with the project for senior stakeholders. With that in mind, I'm wondering whether changing the color of the contextual nudges (as shown in the Figma file) is something we should hold off on for now? While removing the color of the contextual nudge might draw more attention to the upsell nudge, it could also lead to less focus on the nudge area in general. The bright red/yellow of the contextual nudges does draw a lot of attention to the nudge area and thus also to the upsell nudges. In summary, my concern would be that changing the colors here would impact the nudge conversion performance and could potentially negatively affect a key indicator for the nav unification performance. Given that nudges won't be in wp-admin anyway, I'm wondering whether this risk is worth taking at this stage or whether we should hold off with tweaking the nudge colors until after nav unification has launched and the metrics are positive. Then we could test the impact of this change in isolation.

@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 28, 2020
@cpapazoglou
Copy link
Contributor Author

These are excellent points @frontdevde! Addressed here 2ac093c .

Copy link
Contributor

@frontdevde frontdevde left a comment

Choose a reason for hiding this comment

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

The alignment and text wrapping adjustments are a nice improvement over what we currently have. With the demo coming up, I'm happy to approve the PR as is 👍

The nudge colour change is as per design, so if that's the route we want to go, that's fine by me. As stated above my only concern here is that as a result of changing the colour, the nudge conversion might drop which is a key stakeholder concern (see p2-pbAPfg-103). It'd be unfortunate if that were to negatively impact the performance of the whole project.

@cpapazoglou cpapazoglou merged commit 6a03793 into master Oct 29, 2020
@cpapazoglou cpapazoglou deleted the fix/notices-styles-in-unified-sidebar branch October 29, 2020 08:49
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 29, 2020
@cpapazoglou
Copy link
Contributor Author

@sfougnier , @lcollette I have merged this since the nudge colour is as per design and some alternatives seemed a little of for me.

Now Design Alternative 1 Alternative 2
image image image image

We can revisit this decision as you like.

@ghost
Copy link

ghost commented Oct 29, 2020

Agreed @frontdevde! Is this the nudge style you're referring to?

desktop - contextual nudge

@cpapazoglou
Copy link
Contributor Author

Yes @sfougnier , how did you get that Screenshot?

@ghost
Copy link

ghost commented Oct 29, 2020

Oh sorry @cpapazoglou ! I must have missed your comment before mine. My personal vote would be for Alternative 2, as the alignment of the icon matches the nav items below, and there is less awkward padding caused by the block of yellow.

image

@cpapazoglou
Copy link
Contributor Author

No worries @sfougnier!

@davemart-in could you weigh in?

@davemart-in
Copy link
Contributor

davemart-in commented Oct 30, 2020

@cpapazoglou as a reminder, in our meeting yesterday we decided to bump the width of the left nav out to ~272px (based on @lcollette and @sfougnier recommendation). I imagine that will play into alignment of things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Calypso & wp-admin Navigation All navigation in Calypso and wp-admin, and the unified transitions between the two.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nav Unification: Check domain notices
5 participants