WordPress.org

Make WordPress Core

Opened 12 months ago

Closed 7 months ago

Last modified 6 months ago

#27440 closed task (blessed) (fixed)

Improvements to Plugin Installer modals

Reported by: paulwilde Owned by: helen
Milestone: 4.0 Priority: normal
Severity: normal Version: 3.8
Component: Plugins Keywords: needs-patch
Focuses: ui, administration Cc:

Description

I've made this ticket as broad as possible as it could serve as a main ticket for improvements discussed in #26952.

One idea that I've explored is having the main plugin banner integrated into the modal window.

Here's a run-down of each attachment:

plugin-header-image-none.png - This is how the modal would look if the plugin doesn't have a banner image. This is pretty much how the modal looks now should the changes as discussed in #26952 be made.

plugin-header-image-full.png - This is how the modal would look if the plugin supports a banner image. Incidentally the banner for Jetpack works rather nicely for this design, this wouldn't be the case for all banners. The close icon would change colour (light or dark) based on its background. There's popular solutions to achieve this such as BackgroundCheck.

plugin-header-image-alt-full.png - This is a version which has a more abstract background where the title and tab buttons would be hard to read. I've added a gradient behind the title/tab to make them more readable.

plugin-header-image-alt-reduced.png - The banner takes up a lot of space in relation to the actual content of the banner, so I have the idea that the banner height would reduce as you scroll through the contents of each tab. Google+ used to use this idea with their huge profile cover photos if you don't understand what I mean.

Attachments (32)

plugin-header-image-none.png (283.2 KB) - added by paulwilde 12 months ago.
plugin-header-image-full.png (475.6 KB) - added by paulwilde 12 months ago.
plugin-header-image-alt-full.png (679.5 KB) - added by paulwilde 12 months ago.
plugin-header-image-alt-reduced.png (538.6 KB) - added by paulwilde 12 months ago.
27440-return-banners.diff (643 bytes) - added by tellyworth 9 months ago.
Asks the API to include the 'banners' item. Doesn't do anything with it yet.
27440-return-and-display-banners.diff (1.5 KB) - added by stephdau 8 months ago.
This builds on tellyworth's work and conditionally adds banners as background. See screenshot of the same name.
27440-return-and-display-banners.png (916.7 KB) - added by stephdau 8 months ago.
Current output of 27440-return-and-display-banners.diff
27440-return-and-display-banners.2.diff (3.0 KB) - added by stephdau 8 months ago.
Getting closer to the display in the plugins directory, with title styling and vignette. See https://cloudup.com/cVoOOf8Y9Tl
Screen Shot 2014-07-02 at 16.13.30.png (137.5 KB) - added by iseulde 8 months ago.
Screen Shot 2014-07-02 at 17.05.58.png (87.6 KB) - added by iseulde 8 months ago.
Screen Shot 2014-07-02 at 18.33.33.png (285.2 KB) - added by iseulde 8 months ago.
27440-return-and-display-banners.3.diff (4.9 KB) - added by stephdau 8 months ago.
Update to info patch, now also adds a contributors listing, as well as a donate link, as per wp.org version
27440-return-and-display-banners.4.diff (5.0 KB) - added by stephdau 8 months ago.
Similar to 27440-return-and-display-banners.3.diff, but removes colons after "Contributors", and does not attempt to generate a profile link from a username if none is returned for a contributor (does not always work).
27440-return-and-display-banners.5.diff (5.5 KB) - added by michalzuber 8 months ago.
Same as 27440-return-and-display-banners.4.diff just changed Thickbox close design in wp-admin/css/common.css due different banner colors
27440-return-and-display-banners.6.diff (6.8 KB) - added by stephdau 8 months ago.
Moves the static styles from their experimental location inline to wp-admin/css/common.css. Also introduces a smaller screens mode for banner display: https://cloudup.com/cKMBUfgXefg
27440-return-and-display-banners.7.diff (8.5 KB) - added by stephdau 8 months ago.
This patch keeps also adds reviews ratings display, originally provided as patch to #22599, and switches the contributors items to be displayed inline-block, in the spirit of saving some vertical scrolling when lots are present.
27440-return-and-display-banners.8.diff (8.7 KB) - added by stephdau 8 months ago.
Same as 27440-return-and-display-banners.7.diff but with extra 10px top margin for contributors list, to ensure a nice display on large and small screens alike.
27440-return-and-display-banners.9.diff (8.7 KB) - added by stephdau 8 months ago.
Same as 27440-return-and-display-banners.9.diff, but links reviews "graph" to plugin-install.php?tab=plugin-information&plugin=json-rest-api&section=reviews, for reviews to be viewed in the same UX, not on wp.org's, within the iframe.
27440-return-and-display-banners.10.diff (8.7 KB) - added by stephdau 8 months ago.
Same as 27440-return-and-display-banners.9.diff, but uses $api->slug, not a hardcoded value when linking to the reviews tab. Also check for array_sum( (array) $api->ratings ) > 0 before echoing.
27440-return-and-display-banners.11.diff (9.3 KB) - added by stephdau 8 months ago.
Same as last patch, but no longer displays the "Reviews" tab if none are listed.
27440-return-and-display-banners.12.diff (9.5 KB) - added by stephdau 8 months ago.
Brings the tab design more in line with the original mockups, overlaying the banner, if present, with a slight opacity. See https://cloudup.com/ixPqjOlq9rc
27440-return-and-display-banners.13.diff (10.5 KB) - added by stephdau 8 months ago.
In which we only show the fyi box in the description section, on smaller screens
27440-return-and-display-banners.13-alt.diff (13.2 KB) - added by stephdau 8 months ago.
JS-less alternative to 27440-return-and-display-banners.13.diff, by moving the fyi box underneath the section-holder box, instead of above it. No other changes involved.
27440-changeset-29041.diff (851 bytes) - added by michalzuber 8 months ago.
Added braces and reuse the tab variable in if condition.
27440.patch (32.8 KB) - added by iseulde 8 months ago.
27440.1.diff (33.7 KB) - added by michalzuber 8 months ago.
Reuse defined var tab in if condition. Replaced preventDefault with return false to be similiard to the other anchor click functions.
27440.2.diff (507 bytes) - added by michalzuber 8 months ago.
Added colors, props melchoyce
extra long titles.png (89.3 KB) - added by Clorith 8 months ago.
27440-scrolling-content.diff (2.4 KB) - added by tellyworth 7 months ago.
Incomplete - eliminate separate scrolling for content and fyi
27440-scrolling-content.2.diff (3.2 KB) - added by stephdau 7 months ago.
Builds on tellyworth's 27440-scrolling-content.diff, matching the header and making the whole thing scroll altogether.
27440-scrolling-content.3.diff (4.0 KB) - added by stephdau 7 months ago.
Improves upon 27440-scrolling-content.2.diff and fixes some of the issues listed in https://core.trac.wordpress.org/ticket/27440#comment:70
27440-scrolling-content.4.diff (4.2 KB) - added by tellyworth 7 months ago.
Scroll the entire thickbox content, header and all.

Change History (120)

comment:1 @iseulde12 months ago

  • Focuses ui administration added

These look great! Reminds me a bit of Facebook cover photos.
I'd make the footer a bit bigger again as well (like the media modal).

comment:2 @SergeyBiryukov12 months ago

  • Version changed from trunk to 3.8

comment:3 @nacin12 months ago

  • Milestone changed from Awaiting Review to Future Release
  • Type changed from enhancement to task (blessed)

Let's do this for 4.0, for sure. Love it.

I would like to see plugin-header-image-full.png but ideally with the same styling as the plugin header on WordPress.org. That would mean a vignette, plus the text should be placed in the same position and use the same style. The gray that appears above the tabs could also be used in a similar fashion.

Note that an "X" looks great for these screenshots but it may not work well otherwise. That said, a white X if positioned over the vignette might stand up to color contrast requirements.

comment:4 @ircbot9 months ago

This ticket was mentioned in IRC in #wordpress-dev by avryl. View the logs.

comment:5 @SergeyBiryukov9 months ago

  • Milestone changed from Future Release to 4.0

comment:6 @ircbot9 months ago

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.

comment:7 @tellyworth9 months ago

I added a new 'banners' element to the plugin info API to help with this. It's not returned by default, I made it optional until it's a definite thing.

The new patch I added will request the banners element when it calls the plugin_information API. It doesn't do anything with the banner yet, the patch is just to demonstrate how to request it.

Last edited 9 months ago by tellyworth (previous) (diff)

@tellyworth9 months ago

Asks the API to include the 'banners' item. Doesn't do anything with it yet.

comment:8 @mordauk8 months ago

I really love this.

@stephdau8 months ago

This builds on tellyworth's work and conditionally adds banners as background. See screenshot of the same name.

@stephdau8 months ago

Current output of 27440-return-and-display-banners.diff

comment:9 @stephdau8 months ago

Added 27440-return-and-display-banners.diff and 27440-return-and-display-banners.png.

avryl kindly offered to help me with the styling (CSS isn't my specialty :) ).

@stephdau8 months ago

Getting closer to the display in the plugins directory, with title styling and vignette. See https://cloudup.com/cVoOOf8Y9Tl

comment:10 @ircbot8 months ago

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.

comment:11 @iseulde8 months ago

What's the reason for not doing something like plugin-header-image-alt-full.png again? With the tabs overlaying the image.

comment:12 @iseulde8 months ago

I feel sorry for BuddyPress. :)

comment:13 follow-up: @paulwilde8 months ago

Maybe an idea to prevent issues like the above is to have the option for a separate image which is tailored for the plugin installer, and then the current ones are only used in the plugin directory?

It would allow a lot more flexibility rather than trying to basically shoe-horn them in, as the above modal is 830px in width yet the banners used in the plugin directory are 772px so the banners won't look as sharp due to them being scaled up.

comment:14 @stephdau8 months ago

BuddyPress: actually, that's going to be helpful, because they figured out the exact alignment for us. So all we have to do is match that to get exactly what's on the wp.org directory. :D

https://i.cloudup.com/HaMkphy-he.png

comment:15 @stephdau8 months ago

plugin-header-image-alt-full.png: because the tabs over the header look nice when over cherry picked banners with contrast and things, but not most of the time. Nothing a subtle 40-60% opacity white background for the tabs wouldn't fix though. All up for play, haven't focused on the tabs at the moment, beyond pushing them down.

Last edited 8 months ago by stephdau (previous) (diff)

comment:16 in reply to: ↑ 13 @iseulde8 months ago

I wouldn't try to make it work. They shouldn't do something like that, because it will not work when adjusting the image for smaller screens.

Working on an alternative styling.

comment:17 @stephdau8 months ago

paulwilde: hear hear, indeed. Nacin and I discussed that this weekend in Seattle, and the downside is to put yet another "requirement" (though we'd make it optional, of course), or burden on the plugins authors. Another way to look at it, if we're sticking with a modal, is to reduce said modal's size by those same 28 pixels. It's not like we're not going to want to fit in smaller spaces anyway (mobile anyone?). :)

But the core is: I worked on finding the right spot for the code so far, and the CSS/markup are all up in the air. Sky's the limit, now that we know that showing the banner, as a concept, is a viable thing indeed. Nothing set in stone.

Last edited 8 months ago by stephdau (previous) (diff)

comment:18 @stephdau8 months ago

avryl: buddypress: worksforme

comment:19 @iseulde8 months ago

@stephdau On mobile?

comment:20 @iseulde8 months ago

This is what it currently looks like in the plugin repo. :)

comment:21 @stephdau8 months ago

mobile: you know if we don't think of it, we'll be asked to. Might as well think of it early. :)

comment:22 @stephdau8 months ago

FYI: I also don't think we must limit ourselves to the modal, when/if deciding to reach some parity in display between it and the plugins directory. If a better way to present the information involves changing how it is displayed on wordpress.org, I'm sure we can make that happen too.

comment:23 @iseulde8 months ago

I wonder if it wouldn't be better to stay away from the image entirely and make the entire modal scrollable instead of the two sections. We could pin the title and tabs when scrolling past the image. We also could move the install button up to free up some space. And the close button could be moved outside the modal to ensure enough contrast?

comment:24 @iseulde8 months ago

Maybe it's also time to redo this in backbone? :)

comment:25 @stephdau8 months ago

all scrollable with pinned title:: that was my next thing to play with too! :) There have been mentions of also pinning part of the banner (previous Google+ style), but I've been noticing that every developer uses the banner differently. Some of them having abstract pictures (like contact form 7 in the original example), while others rely on the image's ratios to lay things out (like BuddyPress, or your own WP Front End Editor plugin). So with that in mind, I'm not sure it's worth the loss of actual UX real-estate, and would prefer to scroll it out of view once the user starts to interact (scrolling) with the modal too.

backbone: that's the general sentiment. We've also floated around the idea of not having a modal, favoring going to a full page instead (the current iframe is one, minus the navigation, essentially).

Sending the user to a diff page could have 2 main advantages:

  • allows the user to open multiple plugins details, in multiple tabs, which is a common usage pattern when searching for things (allows for comparison, etc).
  • easy to link to from anywhere (eg: installed plugins list could use a link to the details too), without having to load JS just in case the user wants to, which means a "lighter" page on slower devices.

Moving to backbone would:

  • make it easy to have next/prev buttons without page reload (based on in-page data, until more is needed)
  • bring it in line with what's being done for the media grid, and has already been done for the media uploader and themes.

Thoughts?

Last edited 8 months ago by stephdau (previous) (diff)

comment:26 @iseulde8 months ago

Yes, we should probably scroll the whole image out of view.

Maybe we could go for something in between like the details "modal" for themes? It doesn't overlap the navigation and has its own links, so it's kind of a separate page. I've noticed you can't open them in a new tab though, so I'll submit a ticket for that.

comment:27 follow-up: @stephdau8 months ago

So, the only issue I have with a move to backbone is timeframe for 4.0.

At this time, with beta next week, I'm not sure if it's reasonable to make the move to it now, since it's essentially a rewrite. That might even be better as a feature-as-a-plugin, like the THX38 (themes) and Media Grid projects.

I think our best bet at this time, for 4.0 specifically, is to stretch the current framework (Thickbox and iframed page) a tad longer as we validate our theory that having a more complete plugins details page is valuable to users in the first place. We can converse and iterate rapidly, as we're doing now, make sure it makes 4.0, get feedback on the additional data we'll be presenting (banner, reviews, etc), then aggregate that and move to something like backbone.

This is, of course, taking in consideration my own backbone experience, which is not-so-bad, but also not-optimum at the moment. Having someone more agile with it for the next week could change my mind on the tech front, but I still think we'd have more testing overhead than sticking with what we have, and improving it, for now.

Wadya think?

Last edited 8 months ago by stephdau (previous) (diff)

comment:28 in reply to: ↑ 27 @stephdau8 months ago

Replying to stephdau:

So, the only issue I have with a mov to backbone is timeframe for 4.0.

We're still free to make the UI look closer to the themes details one, of course, within the existing framework.

comment:29 @iseulde8 months ago

Sounds good!

Is the author's gravatar currently provided by the API?

comment:30 @stephdau8 months ago

Gravatar: not that I can see at the moment. Only author (markup) and author_profile (URL):

["author"]=>
  string(66) "<a href="https://corepressthis.wordpress.com/">Press This Team</a>"
["author_profile"]=>
  string(33) "//profiles.wordpress.org/stephdau"

It's unlikely to be hard to add to the API response, if you want it.

Last edited 8 months ago by stephdau (previous) (diff)

comment:31 follow-up: @iseulde8 months ago

Yes! :)
But we need all the authors of the plugin, not just one.

comment:32 in reply to: ↑ 31 @stephdau8 months ago

Replying to avryl:

Yes! :)
But we need all the authors of the plugin, not just one.

There's a contributors array, but its current format doesn't lend itself to having more data added to it, and would therefore need to be changed. I see the API is versioned (http://api.wordpress.org/plugins/info/1.0/), so maybe we can leverage that to not get into backwards compatibility issues.

["contributors"]=>
  array(5) {
    ["michael-arestad"]=>
    string(0) ""
    ["stephdau"]=>
    string(33) "//profiles.wordpress.org/stephdau"
    ["georgestephanis"]=>
    string(40) "//profiles.wordpress.org/georgestephanis"
    ["folletto"]=>
    string(33) "//profiles.wordpress.org/folletto"
    ["melchoyce"]=>
    string(34) "//profiles.wordpress.org/melchoyce"
  }

Or we could instead have a 'gravatars' array, also using the contrib's username as a key.

Will talk about that with the core crew who maintains the wp.org side of the API to confirm what's best.

Is there any other plugins data you thought would need exposing while playing around?
Things you see displayed on the plugins directory, but not in the modal (and that make sense to be there).

comment:33 follow-up: @iseulde8 months ago

The donate button maybe?

comment:34 in reply to: ↑ 33 @stephdau8 months ago

Replying to avryl:

The donate button maybe?

Yup, good idea, and we have the donate_link as part of the API response already.

 ["donate_link"]=>
  string(38) "http://wordpressfoundation.org/donate/"

FYI: here's an example of a full response, here for the Press This plugin: https://cloudup.com/cLxbE5Tgm50

Last edited 8 months ago by stephdau (previous) (diff)

comment:35 @stephdau8 months ago

Just FYI: asked about the best way to provide the gravs (and if ok to return, since a 3rd-party service, technically) to the wp.org admins. But do keep in mind we're on July 4th, so reply might take a while longer then usual. :)

comment:36 @stephdau8 months ago

Check what I found right here on core.trac.wordpress.org, for its own integration with Gravatar:

https://wordpress.org/grav-redirect.php?user=stephdau&s=36

That translates/redirects to my Gravatar img, with only my wp.org username, which we happen to have for all contributors, and without leaking a user's email address through the API.

https://secure.gravatar.com/avatar/5b8d74a711e183850bd70ccdd440d15e?s=36&d=retro

So no need to have the API return Gravatars after all, we can simply use that URL pattern in the modal (be it on the client or server sides).

comment:37 follow-up: @iseulde8 months ago

Oh that's cool! But it's slower that linking it directly... Is providing an md5'ed email address through the API considered leaking? :)

@stephdau8 months ago

Update to info patch, now also adds a contributors listing, as well as a donate link, as per wp.org version

comment:38 in reply to: ↑ 37 @stephdau8 months ago

Replying to avryl:

Oh that's cool! But it's slower that linking it directly... Is providing an md5'ed email address through the API considered leaking? :)

Nope, but not returning them bypasses the versioning issue, and keeps the API response lighter (albeit those would likely be an optional param, like banners are at the moment).

I've used the wp.org grav redirect trick in my latest patch (27440-return-and-display-banners.3.diff), we'll see if speed is an issue, and go from there if it is.

Last edited 8 months ago by stephdau (previous) (diff)

@stephdau8 months ago

Similar to 27440-return-and-display-banners.3.diff, but removes colons after "Contributors", and does not attempt to generate a profile link from a username if none is returned for a contributor (does not always work).

@michalzuber8 months ago

Same as 27440-return-and-display-banners.4.diff just changed Thickbox close design in wp-admin/css/common.css due different banner colors

comment:39 @stephdau8 months ago

Thanks michalzuber! I was wondering where to do that and had kept looking that up for later. :)

@stephdau8 months ago

Moves the static styles from their experimental location inline to wp-admin/css/common.css. Also introduces a smaller screens mode for banner display: https://cloudup.com/cKMBUfgXefg

comment:40 @stephdau8 months ago

Added a new patch to tweak and move the styles to the appropriate (hopefully) static file.

@stephdau8 months ago

This patch keeps also adds reviews ratings display, originally provided as patch to #22599, and switches the contributors items to be displayed inline-block, in the spirit of saving some vertical scrolling when lots are present.

@stephdau8 months ago

Same as 27440-return-and-display-banners.7.diff but with extra 10px top margin for contributors list, to ensure a nice display on large and small screens alike.

comment:41 @stephdau8 months ago

Latest patch (27440-return-and-display-banners.8.diff) also includes the review ratings details worked on by tellyworth in #22599, as well as refines the contributors listing display as inline-block to save on vertical scrolling when lots are listed.

https://cloudup.com/cUM4szz4ugS

@stephdau8 months ago

Same as 27440-return-and-display-banners.9.diff, but links reviews "graph" to plugin-install.php?tab=plugin-information&plugin=json-rest-api&section=reviews, for reviews to be viewed in the same UX, not on wp.org's, within the iframe.

@stephdau8 months ago

Same as 27440-return-and-display-banners.9.diff, but uses $api->slug, not a hardcoded value when linking to the reviews tab. Also check for array_sum( (array) $api->ratings ) > 0 before echoing.

@stephdau8 months ago

Same as last patch, but no longer displays the "Reviews" tab if none are listed.

comment:42 @stephdau8 months ago

27440-return-and-display-banners.[9->11].diff improves upon the reviews tab display (or hiding).

@stephdau8 months ago

Brings the tab design more in line with the original mockups, overlaying the banner, if present, with a slight opacity. See https://cloudup.com/ixPqjOlq9rc

comment:43 @stephdau8 months ago

27440-return-and-display-banners.12.diff​ brings the tab design more in line with the original mockups, overlaying the banner, if present, with a slight opacity.

This also saves 35px worth of height real-estate. :)

See https://cloudup.com/cSzdC881YyR

https://i.cloudup.com/Q9lpXCbvux.png

https://i.cloudup.com/d5fzj1sTE0.png

Last edited 8 months ago by stephdau (previous) (diff)

@stephdau8 months ago

In which we only show the fyi box in the description section, on smaller screens

comment:44 @stephdau8 months ago

27440-return-and-display-banners.13.diff adds a tiny bit of JS handling to tab switching, so we only show the FYI box in the mandatory description section, on smaller screens (< 830px, as other such handling in the modal).

If not doing so, the user often did not see a difference in the content section when switching tabs, as the FYI box could take all of their vertical display area, therefore missing the content underneath it.

The FYI box is just that: an FYI. So it's not necessary to show it everywhere, unless we have the real estate to do so appropriately.

See results in https://cloudup.com/cHffCf1t8hg

Last edited 8 months ago by stephdau (previous) (diff)

comment:45 @stephdau8 months ago

Original mockup by paulwilde:

https://core.trac.wordpress.org/raw-attachment/ticket/27440/plugin-header-image-alt-full.png

Current state of the modal with latest patch applied:

https://i.cloudup.com/VL-x48N3ua.png

Hope it finds its way into core. :)

comment:46 @stephdau8 months ago

For the record: A JS-less, and likely more reliable way of dealing with the FYI box for smaller screens could also simply be to have the <div class="fyi"> markup after <div id="section-holder" class="wrap">, instead of before it. The existing CSS lays it out in the same way on larger screens, but soft-wraps it underneath the content, instead of above it, at all times.

That, would be my preferred way to go, but I'm unsure if the current markup structure is based on accessibility science/logic or just "random". :)

Thoughts?

@stephdau8 months ago

JS-less alternative to 27440-return-and-display-banners.13.diff, by moving the fyi box underneath the section-holder box, instead of above it. No other changes involved.

comment:47 @stephdau8 months ago

27440-return-and-display-banners.13-alt.diff illustrates the JS-less alternative mentioned in my last comment, over 27440-return-and-display-banners.13.diff

Results: https://cloudup.com/c7SLpilEm_K

comment:48 @stephdau8 months ago

  • Keywords has-patch added

comment:49 @ircbot8 months ago

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.

comment:50 @helen8 months ago

In 29040:

Improvements to the plugin information modal:

  • Show the banner image when available.
  • Show contributors and ratings breakdown in the FYI box.
  • Show reviews in a tab.

props stephdau, tellyworth, paulwilde, michalzuber. see #27440. fixes #19784, #22599, #26202.

comment:51 @helen8 months ago

Next up: responsive and accessibility considerations (keyboard, screen reader).

comment:52 @helen8 months ago

In 29041:

Autoprefixer for [29040]. see #27440.

comment:53 @iseulde8 months ago

Why is the banner stretched to an unnatural width? Shouldn't the modal be smaller instead?

Also, no braces? :(
http://make.wordpress.org/core/handbook/coding-standards/php/
http://make.wordpress.org/core/handbook/coding-standards/javascript/

@michalzuber8 months ago

Added braces and reuse the tab variable in if condition.

@iseulde8 months ago

comment:54 @iseulde8 months ago

In 27440.patch I added braces, spaces etc. for the whole file, not just the code added, removed some redundant inline CSS, made the modal a bit smaller based on the width of the banner and removed a lost brace in background-image: url(<?php echo $high ?>});.

comment:55 @iseulde8 months ago

Also reduced the amount of @media queries, because that was a bit too much. :)

@michalzuber8 months ago

Reuse defined var tab in if condition. Replaced preventDefault with return false to be similiard to the other anchor click functions.

comment:56 @stephdau8 months ago

Thanks avryl and michalzuber. As you ca tell, I specialize in "making stuff happen", not the finer details. ;)
Kidding, was pressed for time to make the beta. But big thanks nonetheless. :)

comment:57 @michalzuber8 months ago

Nice job stephdau, that's why are we here, to watch each others back ;)

comment:58 follow-ups: @ocean908 months ago

In 29125:

Improvements to the plugin information modal:

  • Whitespace/braces cleanup for [29040].
  • Reduce width of modal to 792px, which is the default banner size.
  • Reduce banner height when screen height is smaller than 500px.
  • Remove inline CSS.
  • Re-use tab var in JS.
  • Encode ampersands in links.

props avryl, michalzuber, ocean90.
see #27440.

comment:59 in reply to: ↑ 58 @ocean908 months ago

Replying to ocean90:

792px => 772px

comment:60 in reply to: ↑ 58 @stephdau8 months ago

Replying to ocean90:

In 29125

Very cool. Nice work all.

comment:61 @melchoyce8 months ago

Just noticed the plugin modal close icon doesn't use the color scheme highlight color, but defaults to the default scheme's blue: https://cloudup.com/charWUVuf5U

The theme browser uses the correct color, for reference: https://cloudup.com/cNwvVqaXG-e

@michalzuber8 months ago

Added colors, props melchoyce

comment:62 @ircbot8 months ago

This ticket was mentioned in IRC in #wordpress-dev by stephdau. View the logs.

@Clorith8 months ago

comment:63 follow-up: @Clorith8 months ago

Long titles go a bit wonky, as seen from the screenshot above.

comment:64 @ircbot8 months ago

This ticket was mentioned in IRC in #wordpress-dev by Clorith. View the logs.

comment:65 in reply to: ↑ 63 @melchoyce8 months ago

Replying to Clorith:

Long titles go a bit wonky, as seen from the screenshot above.

Related: #28883

comment:66 follow-up: @nacin8 months ago

I would like to see some changes to how header images are rendered: The image should not have tabs overlaying it and it should be presented exactly how it is on the plugin directory. As in, as a baseline, BuddyPress should look OK and Akismet should not have anything cut off. That's not to say we won't change how these work in the future, but I don't think that's what we should be doing for 4.0.

A major issue with this new design is that the header is sticky and takes up a lot of room. If it remains sticky, what I would like to see is a min-height be required. On my 11" MacBook Air, it's definitely cramped. I'd rather see no header image at all in a bit of a "compact" mode.

Additionally, the left and right columns scroll separately, which is very awkward. Making it scroll together would help. But what I'm wondering is if we just made the whole thing scroll (including the header), we'd be in better shape. We'd also be able to avoid using a min-height to hide the header (at least maybe until we get to only a few hundred pixels). If we then did continuous scrolling through the sections instead of tabs, that would mean we don't need to dock any tabs, either.

comment:67 in reply to: ↑ 66 @celloexpressions8 months ago

Replying to nacin:

Additionally, the left and right columns scroll separately, which is very awkward. Making it scroll together would help. But what I'm wondering is if we just made the whole thing scroll (including the header), we'd be in better shape. We'd also be able to avoid using a min-height to hide the header (at least maybe until we get to only a few hundred pixels). If we then did continuous scrolling through the sections instead of tabs, that would mean we don't need to dock any tabs, either.

Also, we should disable scrolling on the background page behind the modal (as has been proposed/is being done for media modal).

@tellyworth7 months ago

Incomplete - eliminate separate scrolling for content and fyi

comment:68 @ocean907 months ago

  • Keywords needs-patch added; has-patch removed

comment:69 @ircbot7 months ago

This ticket was mentioned in IRC in #wordpress-dev by stephdau. View the logs.

@stephdau7 months ago

Builds on tellyworth's 27440-scrolling-content.diff, matching the header and making the whole thing scroll altogether.

comment:70 @stephdau7 months ago

Back from vacation. :)

Added 27440-scrolling-content.2.diff, keeping on building on tellyworth's work in 27440-scrolling-content.diff, to implement nacin's latest requests.

The new patch:

  • matches the header more with the directory's (no overlaid tabs, BuddyPress and Akismet baselines achieved)
  • makes header+tabs+content+fyi scrollable, by including the 2 former inside the new scrollable div Alex added.

Known issues to deal with (see last 2 screenshot in link below):

  • title h2 doesn't scroll along with the rest (fixed pos?)
  • the "install now" footer overlay scrolls when one reaches the bottom of the content, same goes for the background content (latter also mentioned by celloexpressions above).
  • something along the way (likely the scrollable wrapper div) made the content column too narrow in mobile views, ad should be widened to use the whole width.

Screenshots: https://cloudup.com/co8jWI3qxgg

Have to go for a while, but will get in touch with Alex once back, and keep going at it with him.

Last edited 7 months ago by stephdau (previous) (diff)

@stephdau7 months ago

Improves upon 27440-scrolling-content.2.diff and fixes some of the issues listed in https://core.trac.wordpress.org/ticket/27440#comment:70

comment:71 @stephdau7 months ago

Added 27440-scrolling-content.3.diff to address some of the issues listed in my last comment.

  • Title H2 is no longer static (when there's a banner), and scrolls along with the header.
  • The install now footer/button is now static, and no longer scrolls up when a user reaches the bottom of the srollable content.
  • Fixed the content width issue in narrow screens.
  • Also makes sure bottom-most text does scroll above the install now footer, when reached, instead of stopping underneath it.

Screenshots: https://cloudup.com/c7PzEF_7t7I

Last edited 7 months ago by stephdau (previous) (diff)

comment:72 follow-ups: @stephdau7 months ago

Still to deal with:

  • Find a way to deal with tabs better, when there are more than the current screen width allows for. Currently wraps, doesn't look clean: https://cloudup.com/cwqyP1ZuTuF
  • Have scrolling not propagate to "background" page when the end of the content is reached inside the iframe
  • Long titles: see #28883 and patches there.
Last edited 7 months ago by stephdau (previous) (diff)

comment:73 in reply to: ↑ 72 ; follow-up: @stephdau7 months ago

Replying to stephdau:

Have scrolling not propagate to "background" page

celloexpressions mentioned in the weekly dev meeting that he'd make a round of all of wp-admin to deal with the issue with all modals at large (thanks Nick!).

Last edited 7 months ago by stephdau (previous) (diff)

comment:74 in reply to: ↑ 72 @stephdau7 months ago

Replying to stephdau:

Long titles: see #28883

Mentioned current patch conflicts there to avoid issues with future/potential commit sequence.

comment:75 in reply to: ↑ 73 @celloexpressions7 months ago

Replying to stephdau:

Replying to stephdau:

Have scrolling not propagate to "background" page

celloexpressions mentioned in the weekly dev meeting that he'd make a round of all of wp-admin to deal with the issue with all modals at large (thanks Nick!).

Oh, and forgot to link the ticket here: #29074. It does require patching thickbox for plugins, I think, since we don't have Backbone/something more structured yet.

comment:76 @stephdau7 months ago

Thanks! Hoping for Backbonification during 4.1, plugins work happened too close to 4.0 beta code freeze. :)

comment:77 @stephdau7 months ago

Long title are now handled. See r29346. Will update our patch in consequence.

comment:78 @stephdau7 months ago

D'oh... I meant background scrolling. Sorry. :)

@tellyworth7 months ago

Scroll the entire thickbox content, header and all.

comment:79 follow-up: @tellyworth7 months ago

New patch is a variation on Stephdau's changes that scrolls the entire contents, header and menu tabs included.

The bottom bar with the Install button still needs work I think. Perhaps better as a fixed footer rather than overlaid like it is now.

comment:80 in reply to: ↑ 79 ; follow-up: @stephdau7 months ago

Replying to tellyworth:

New patch is a variation on Stephdau's changes that scrolls the entire contents, header and menu tabs included.

Uh... I don't get it. :)
My patch (27440-scrolling-content.3.diff) had everything but the bottom install button scrolling too.
And yours (27440-scrolling-content.4.diff) is busting the title positioning on both large and small screens, which I had matched perfectly.

https://cloudup.com/cUGbYC2ctBd

I'm not seeing any difference between your and my patch's behavior, except for the layout busting.
What am I missing?

Last edited 7 months ago by stephdau (previous) (diff)

comment:81 @ircbot7 months ago

This ticket was mentioned in IRC in #wordpress-dev by johnbillion. View the logs.

comment:82 in reply to: ↑ 80 @tellyworth7 months ago

Yeah ignore my latest patch, I just made things worse :)

comment:83 @helen7 months ago

  • Owner set to helen
  • Resolution set to fixed
  • Status changed from new to closed

In 29474:

Plugin details modal:

  • Scroll all content, rather than the dual-pane scrolling awkwardness.
  • Better align the header image and title overlay with the display on WordPress.org, at least at full-width.
  • Move the close button outside of the modal itself, rather than overlay on top of colors we can't predict.

props stephdau, tellyworth, helen. fixes #27440.

comment:84 @helen7 months ago

In 29480:

Plugin details: Remove some CSS rules that were unnecessary and causing issues in Firefox. see #27440.

comment:85 @ocean907 months ago

In 29506:

Plugin details: Escape the donate link.

see #27440.

comment:86 @ircbot7 months ago

This ticket was mentioned in IRC in #wordpress-dev by Clorith. View the logs.

comment:87 @nacin6 months ago

In 29590:

Plugin details: Correctly calculate the height of the right column for plugins without banners.

Comment these magic numbers.

see #27440, [29474].

comment:88 @nacin6 months ago

In 29591:

Plugin details: Adjust plugin title when it does not have a banner.

Smaller, bold, and Open Sans, like other modals. Only use Helvetica Neue when the plugin has a banner.

see [29040], see #27440.

Note: See TracTickets for help on using tickets.