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

Notices: remove site-notice component #1364

Merged
merged 1 commit into from
Dec 11, 2015
Merged

Conversation

artpi
Copy link
Contributor

@artpi artpi commented Dec 8, 2015

Part of notices refactor intitiative:
#888 #1094 #1105 #1313

This component was only used once and didn't add much additional functionality.

Plus, design-wise, after clicking domain name, the user was redirected to domain management, which was kind of confusing.

Design changes

Before:

After:

Why would you put so much text there?

This is actually the most important information in the whole site right now.
I am wondering if it should be put in a permanent global notice, because no matter what people change in Calypso, they wont see anything on the frontend.

And when I was searching for a way to turn off redirection, I had to dig around A LOT.
@mtias: Any suggestions how to make this look good?

Testing

  1. Set site redirect
  2. Open sidebar
  3. Notice the notice (get it?)

@artpi artpi added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR labels Dec 8, 2015
@mtias
Copy link
Member

mtias commented Dec 8, 2015

This is great, thanks, has been on my list for a while. Once we also convert flags into notice compact it would be even better.

@folletto
Copy link
Contributor

folletto commented Dec 8, 2015

Question: do we need to show the full menu below in this case? Otherwise, the entire side column could be a custom designed component for this instance :)

@mtias
Copy link
Member

mtias commented Dec 8, 2015

@artpi I think the original text is fine, the only purpose of this notice is to help distinguish between the site that redirects and the site that it redirects to when you have both in your list.

@mtias
Copy link
Member

mtias commented Dec 8, 2015

I'd keep things as they were, just getting rid of the custom component. And we'll look at any improvements once we work on "flags".

@artpi
Copy link
Contributor Author

artpi commented Dec 8, 2015

Ok, so it will be like this:

But I think in future, we have to make it more usable.
I was looking for a way to turn of redirection for 40 minutes a few days ago :)

@mtias
Copy link
Member

mtias commented Dec 8, 2015

Sounds good. I'm working on flags now.

@mtias
Copy link
Member

mtias commented Dec 8, 2015

Squash and :shipit:

@mtias mtias added Sites [Status] Ready to Merge Components Sidebar and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 8, 2015
@rickybanister rickybanister removed the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Dec 8, 2015
Notices: notices/site-notice->components/notice in current-site

Notices: fix bottom margin in notices displayed in sidebar

So it matches previous design of site-notice class.

Notices: Remove unused css of site-notice

The component is no longer uses and will be removed shortly

Notices: Remove unused site-notice component
@artpi artpi force-pushed the update/notices__site-notice branch from 2ecaea2 to 8c34746 Compare December 11, 2015 15:06
artpi added a commit that referenced this pull request Dec 11, 2015
@artpi artpi merged commit ada00be into master Dec 11, 2015
@artpi artpi deleted the update/notices__site-notice branch December 11, 2015 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants