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.

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

Plugins: update notices to the new format #1677

Merged
merged 1 commit into from
Dec 17, 2015
Merged

Plugins: update notices to the new format #1677

merged 1 commit into from
Dec 17, 2015

Conversation

johnHackworth
Copy link
Contributor

This PR updates notices in /plugins/ to the new format introduced in https://github.com/Automattic/wp-calypso/pull/1603/files

It both updates the 'update' flags inside of each plugin item

Before:
image

After:
image

And the full-width notice error messages:
Before:
image

After:
image

How to test

  1. Go to http://calypso.dev:3000/plugins/
  2. Check that any update flag you have in the plugin list is rendered with the new notice-compact format
  3. Click on "bulk edit"
  4. Select a plugin and do any action that would result in a error (you probably have to have a failing jetpack site for this);
  5. Check the error notice is shown full-width

@johnHackworth johnHackworth 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 Jetpack Plugins labels Dec 16, 2015
@johnHackworth johnHackworth self-assigned this Dec 16, 2015
mtias
mtias reviewed Dec 16, 2015
View reviewed changes
client/my-sites/plugins/plugin-item/plugin-item.jsx
</span>
</div>
<Notice isCompact
showDismiss={ false }
Copy link
Member

Choose a reason for hiding this comment

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

this should be assumed for isCompact

@mtias
Copy link
Member

mtias commented Dec 16, 2015

Looks good!

mtias
mtias reviewed Dec 16, 2015
View reviewed changes
client/my-sites/plugins/plugin-item/plugin-item.jsx
@@ -14,7 +14,7 @@ var CompactCard = require( 'components/card/compact' ),
PluginsActions = require( 'lib/plugins/actions' ),
PluginActivateToggle = require( 'my-sites/plugins/plugin-activate-toggle' ),
PluginAutoupdateToggle = require( 'my-sites/plugins/plugin-autoupdate-toggle' ),
Notice = require( 'notices/notice' ),
Notice = require( 'components/notice' ),
Copy link
Member

Choose a reason for hiding this comment

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

\o/

@rickybanister
Copy link

I think we should not set the bottom margin of the regular sized notice to 0. When it pushes down the next card it makes it obvious that it is attached to the top one.

@johnHackworth
Copy link
Contributor Author

@rickybanister do you mean leaving it like this?

image

@johnHackworth johnHackworth force-pushed the add/plugins-error-notices branch 2 times, most recently from 9d90809 to 4dba5ab Compare December 16, 2015 16:43
@johnHackworth
Copy link
Contributor Author

I have removed the margin-bottom: 0 to see how it works. Also, now all the small notifications under the plugin name uses a notice compact, so we can discard a lot of custom css and avoid size jump when the contents in that area changes:

image

In the above screencap, 'A newer version ... ', 'Last updated...', and 'Deactivatig' are all <Notice compact>.

@johnHackworth
Copy link
Contributor Author

(also, I adjusted the notice-compact height (to the values they will have as default when #1676 is merged) & the line-height of the plugin name to make them fit into the plugin icon height)

mtias
mtias reviewed Dec 16, 2015
View reviewed changes
client/my-sites/plugins/plugin-item/style.scss
}
}

.notice.is-in-progress,
Copy link
Member

Choose a reason for hiding this comment

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

I think these should use some of the default statuses, is-info?

Copy link

Choose a reason for hiding this comment

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

I'd agree with that. The standard 'Last updated' text doesn't need a status, but when we are updating or deactivating we could make it more visible.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah.

@johnHackworth johnHackworth force-pushed the add/plugins-error-notices branch from 4dba5ab to 7212f5c Compare December 16, 2015 17:45
@johnHackworth
Copy link
Contributor Author

new version!

image

the status flags now use regular notices (no transparent background) and the standing texts there ('last updated ... ') are not notices at all

mtias
mtias reviewed Dec 16, 2015
View reviewed changes
client/my-sites/plugins/plugin-item/style.scss
.plugins-list &.is-compact:last-child {
margin-bottom: 0;

.notice.is-compact {
Copy link
Member

Choose a reason for hiding this comment

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

All of this should be removed once #1676 lands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@rickybanister
Copy link

@mtias I know we can throw a custom icon in notices, but can we make it have no icon? For the 'Activating' notice, it doesn't seem necessary to show the little (i)

@mtias
Copy link
Member

mtias commented Dec 16, 2015

If we think it's something we'll do often, we can add a prop. Otherwise just a .your-component .notice__icon { display: none; } :D

@johnHackworth johnHackworth force-pushed the add/plugins-error-notices branch 2 times, most recently from 64554e3 to da4e509 Compare December 16, 2015 20:24
@johnHackworth
Copy link
Contributor Author

I removed the non-needed notices styles in this PR and removed the icon in the is-info notices inside of plugin-item:

image

@rickybanister
Copy link

Perfect, looking really sharp. Ship ship ship

@enejb
Copy link
Member

enejb commented Dec 16, 2015

I wasn't able to see any of the inline error notices any more. Were they removed?

@johnHackworth
Copy link
Contributor Author

@enejb they are shown only if you are in bulk edit mode, for some reason ... in this PR #1554 I'm changing them to be shown always

@enejb enejb force-pushed the add/plugins-error-notices branch from da4e509 to 4cbaa64 Compare December 17, 2015 01:31
@enejb
Copy link
Member

enejb commented Dec 17, 2015

I just pushed a rebase here. I think this is ready to merge!

@enejb enejb removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 17, 2015
johnHackworth added a commit that referenced this pull request Dec 17, 2015
@johnHackworth
Merge pull request #1677 from Automattic/add/plugins-error-notices
a211cd5 
Plugins: update notices to the new format
@johnHackworth johnHackworth merged commit a211cd5 into master Dec 17, 2015
@johnHackworth johnHackworth deleted the add/plugins-error-notices branch December 17, 2015 09:23
@lancewillett lancewillett 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 Oct 19, 2016
@snyk-bot snyk-bot mentioned this pull request Aug 21, 2020
Open
This was referenced Nov 18, 2020
Open
Open
@snyk-bot snyk-bot mentioned this pull request Apr 22, 2021
Open
@awaisab172 awaisab172 mentioned this pull request Nov 28, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@johnHackworth johnHackworth

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@johnHackworth @mtias @rickybanister @enejb @lancewillett @scruffian