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

Sidebar: fix to restore upsell ads old styling (Free Domain, Upgrade) #20833

Merged
merged 1 commit into from
Dec 14, 2017

Conversation

folletto
Copy link
Contributor

This PR restores the styling of the upgrade banners in the sidebar that were changed as a side effect of #17796.

Before:

screen shot 2017-12-14 at 18 11 04

After:

screen shot 2017-12-14 at 18 06 33

Note: in the screenshot I forced both the banners to be visible, they aren't normally.

The fix isn't amazing, it's just adding a class and overriding the styling. This needs later to me moved to an ad-hoc component for marketing purposes.

To review

  1. Open My Sites
  2. Find a site with a Free Plan and created less than 180 days or change in code the eligibility conditions in the two files.
  3. Check the banner shows up with the right colors.
  4. Check also that other banners preserve the normal styling.

@folletto folletto added [Pri] High Address as soon as possible after BLOCKER issues [Type] Bug labels Dec 14, 2017
@matticbot
Copy link
Contributor

@sararosso
Copy link
Contributor

Thank you for this! Just a tiny note for testing that the site needs to be 2-180 days old for the free-to-paid - the nudge isn't currently shown on D1 sites.

@cristelrossignol
Copy link
Contributor

The nudge is not showing for me
(site is between 2-180 days old and on a free plan)
testingcristel543211.wordpress.com
https://cloudup.com/cPv_77NL_Sl

@aidvu aidvu self-requested a review December 14, 2017 18:13
Copy link
Contributor

@aidvu aidvu left a comment

Choose a reason for hiding this comment

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

LGTM

@aidvu
Copy link
Contributor

aidvu commented Dec 14, 2017

Not a fan of the uppercase. Missing padding-left.

screen shot 2017-12-14 at 18 17 03

Copy link
Contributor

@aidvu aidvu left a comment

Choose a reason for hiding this comment

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

When they are other notices, doesn't look consistent.

@apeatling
Copy link
Member

Agreed, we should make sure all of these are consistent.

@sararosso
Copy link
Contributor

sararosso commented Dec 14, 2017

There's a longer discussion about why these notices should be different than the others on marketingscratchpad - pinging @apeatling and @aidvu with the link for background.

Edited to add the p2 link - [code p2-p7EOS0-442]

@folletto
Copy link
Contributor Author

Not a fan of the uppercase. Missing padding-left.

Both are done on purpose (the letter aligns with the end of the color, if it would align with text it would look off for too much space). They match the old design.

This PR is meant to be a quick fix. Let's get this in, then we have time to create a new component ad hoc that isn't a hack and adjust all the above.

Copy link
Contributor

@aidvu aidvu left a comment

Choose a reason for hiding this comment

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

Let's get this in and iterate. Chances someone sees more than 1-2 notices are very low.

@aidvu aidvu merged commit ced813c into master Dec 14, 2017
@aidvu aidvu deleted the fix/upgrade-site-banners branch December 14, 2017 19:18
@folletto
Copy link
Contributor Author

Thank you 🌟

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Notifications Site notifications. [Pri] High Address as soon as possible after BLOCKER issues [Type] Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants