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

Extension version text on Extensions page #1122

Merged
merged 9 commits into from
Oct 12, 2016
Merged

Conversation

dashaluna
Copy link
Contributor

@dashaluna dashaluna commented Sep 28, 2016

Fixes #1098

TODO

@codecov-io
Copy link

codecov-io commented Sep 28, 2016

Current coverage is 37.93% (diff: 100%)

Merging #1122 into master will not change coverage

@@             master      #1122   diff @@
==========================================
  Files            31         31          
  Lines          2151       2151          
  Methods         256        256          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits            816        816          
  Misses         1335       1335          
  Partials          0          0          

Powered by Codecov. Last update e03cf9e...7cee914

@willmot
Copy link
Contributor

willmot commented Sep 28, 2016

Thanks @dashaluna, big improvement. Could you drop a screenshot of what it looks like, want to check the new wording feels right in-situ.

@dashaluna
Copy link
Contributor Author

dashaluna commented Sep 28, 2016

Here is how it looks now:
screen shot 2016-09-28 at 09 50 15

@willmot styling question:

Is there default WP admin styling class that can be adjusted so that we have more space for plugin version area. So that they are on 2 lines instead of 3? Current styling seems to come from wp-admin/css but I couldn't find style guides for it online?

@willmot
Copy link
Contributor

willmot commented Sep 28, 2016

The original design intention of this page was to mimic the design of the add plugins screen in wp-admin (seen here: wp-admin/plugin-install.php?s=query+monitor&tab=search&type=term).

screen shot 2016-09-28 at 10 06 55

It looks like they no longer even show the version so we could actually just drop that completely from plugins which aren't installed or which don't have an update available.

For extensions which are installed and which need updating, let's replace the installed button with an update button. Something like:

screen shot 2016-09-28 at 10 12 54

We should also change the titles to <h3 />'s instead of <h4 />'s and remove the CSS height from the icons (so they're not stretched).

@willmot
Copy link
Contributor

willmot commented Sep 28, 2016

It would obviously be much nicer if we could have much shorter descriptions as well as the cards are all too long and look messy.

…on statuses to mimic WP plugin card functionality: Update now, Active, Activate, Buy Now
@dashaluna
Copy link
Contributor Author

@willmot I've tried to mimic the WP plugin card functionality button as much as possible. So now we have buttons:

  1. Update Now <= installed & update available
  2. Active <= installed & activated
  3. Activate <= installed
  4. Buy Now <= otherwise

Couple of questions:

  1. WP core differentiate between Activate and Network Activate ref: https://github.com/WordPress/WordPress/blob/master/wp-admin/includes/class-wp-plugin-install-list-table.php#L469.
  2. There are capability checks before displaying plugin management buttons ref https://github.com/WordPress/WordPress/blob/master/wp-admin/includes/class-wp-plugin-install-list-table.php#L451 What's our approach here? Shall we do the same?

@dashaluna
Copy link
Contributor Author

and remove the CSS height from the icons (so they're not stretched).

@willmot I can see that rule in the WP native CSS. The icons don't look stretched for me. Are you viewing on big screen? Not sure how to correctly remove this.

@willmot
Copy link
Contributor

willmot commented Sep 28, 2016

Awesome!

  1. WP core differentiate between Activate and Network Activate ref:

I'm not actually sure how the extension page functions when BackUpWordPress is Network Activated, something we should test. In Multisite setups the main backup wp-admin page moves to Network Settings so the Extensions page should too and Extensions should then also be Network Activated, let's leave that for a separate ticket though.

  1. There are capability checks before displaying... What's our approach here? Shall we do the same?

Good catch, we should do the same 👍

@willmot
Copy link
Contributor

willmot commented Sep 28, 2016

The icons don't look stretched for me.

It's just the Windows Azure icon:
screen shot 2016-09-28 at 14 45 28
Should be:
screen shot 2016-09-28 at 14 45 44

Not sure how to correctly remove this.

You'll need to override in our CSS file with a more specific selector.

@willmot
Copy link
Contributor

willmot commented Sep 28, 2016

Could you also start a ticket for simplifying the descriptions. I think we could probably get them all down to a single paragraph or so.

Update
Issue #1123

@dashaluna
Copy link
Contributor Author

@willmot is it a good idea to display sale stats per extension similar to WP ratings? Don't know if it's something we can pull out.

@dashaluna
Copy link
Contributor Author

@willmot I'm a little bit stuck with Update Now functionality. I've copied over the update URL from WP core, but I'm guessing because extensions aren't in the WP plugins repo that's why it doesn't work. What should be the process/URL of updating a plugin?

@dashaluna
Copy link
Contributor Author

Update on the Update Now functionality :) The update URL works correctly, only when the valid licence has been specified prior.

@willmot should we only show Update Now on the Extension page only with valid licence, as this is how it currently works for the main WP plugin page ref: #1126

@willmot
Copy link
Contributor

willmot commented Sep 29, 2016

Yeah good call, we could just add the disabled attribute to the button when the license is invalid.

@dashaluna
Copy link
Contributor Author

@willmot I can't figure out how can I check a license status per extension in the main BWP plugin code. Could you give me a hand with that please?

@pdewouters
Copy link
Contributor

I don't think we want to start checking licenses in the main BackUpWordPress plugin

@dashaluna
Copy link
Contributor Author

@pdewouters then there is no way to disable the "Update Now" button if we don't know the license status. And we don't disable it for expired / invalid license when you click the button it says the plugin is up to date, but the button is still displayed. Also, in the case of expired/invalid license we can notify a user and give a choice to purchase it.

@dashaluna
Copy link
Contributor Author

@willmot re Update Now button and license checks - I think we should still investigate it but create a new ticket for it, not to block this one. This one already gone further then the original issue description. The functionality works, it's just ever so slightly unclear when you're trying to update an extension that's expired.

I will fix the Azure logo and we should push this to review & merge for the next release.

Also, apologies for a mammoth PR, I know it's over the original description, but seemed to make sense to bundle up related functionality. I tried to make every commit as atomic as possible, so you could review each commit for related functionality. Apologies about that!

@willmot
Copy link
Contributor

willmot commented Oct 7, 2016

Awesome Dasha!

I think we should still investigate it but create a new ticket for it

Agreed 👍

I will fix the Azure logo and we should push this to review & merge for the next release.

Great, I think we should do a release next week, we've a few things done now.

Also, apologies for a mammoth PR, I know it's over the original description

No problem, great to see so much stuff improved!

@dashaluna
Copy link
Contributor Author

re Azure image - fixed. I made sure to increase the canvas size to 512x512 as per other logos. Updated on the main site and old log is deleted :shipit:

namespace HM\BackUpWordPress;

include_once( ABSPATH . 'wp-admin/includes/plugin.php' );
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume you're including this because we need access to one of the functions it contains? If so, I think it's best practise to include it just before you use the function and add a docblock comment explaining what it's being included for.

That way future developers will know why it's here and whether it's safe to remove or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dashaluna dashaluna removed their assignment Oct 11, 2016
@dashaluna
Copy link
Contributor Author

@willmot ready for review & merge

@willmot willmot merged commit 5f9af61 into master Oct 12, 2016
@willmot willmot deleted the 1098-extension-version-text branch October 12, 2016 07:14
@willmot
Copy link
Contributor

willmot commented Oct 12, 2016

@dashaluna I'm out today but let's try to release tomorrow.

@katmoody katmoody added this to the 3.6.4 milestone Apr 28, 2017
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