WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 23 months ago

Last modified 22 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 2 years ago.
plugin-header-image-full.png (475.6 KB) - added by paulwilde 2 years ago.
plugin-header-image-alt-full.png (679.5 KB) - added by paulwilde 2 years ago.
plugin-header-image-alt-reduced.png (538.6 KB) - added by paulwilde 2 years ago.
27440-return-banners.diff (643 bytes) - added by tellyworth 2 years 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 2 years 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 2 years ago.
Current output of 27440-return-and-display-banners.diff
27440-return-and-display-banners.2.diff (3.0 KB) - added by stephdau 2 years 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 2 years ago.
Screen Shot 2014-07-02 at 17.05.58.png (87.6 KB) - added by iseulde 2 years ago.
Screen Shot 2014-07-02 at 18.33.33.png (285.2 KB) - added by iseulde 2 years ago.
27440-return-and-display-banners.3.diff (4.9 KB) - added by stephdau 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years ago.
Added braces and reuse the tab variable in if condition.
27440.patch (32.8 KB) - added by iseulde 2 years ago.
27440.1.diff (33.7 KB) - added by michalzuber 2 years 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 2 years ago.
Added colors, props melchoyce
extra long titles.png (89.3 KB) - added by Clorith 2 years ago.
27440-scrolling-content.diff (2.4 KB) - added by tellyworth 2 years ago.
Incomplete - eliminate separate scrolling for content and fyi
27440-scrolling-content.2.diff (3.2 KB) - added by stephdau 2 years 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 2 years 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 23 months ago.
Scroll the entire thickbox content, header and all.

Change History (120)

#1 @iseulde
2 years 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).

#2 @SergeyBiryukov
2 years ago

  • Version changed from trunk to 3.8

#3 @nacin
2 years 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.

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


2 years ago

#5 @SergeyBiryukov
2 years ago

  • Milestone changed from Future Release to 4.0

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


2 years ago

#7 @tellyworth
2 years 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 2 years ago by tellyworth (previous) (diff)

@tellyworth
2 years ago

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

#8 @mordauk
2 years ago

I really love this.

@stephdau
2 years ago

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

@stephdau
2 years ago

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

#9 @stephdau
2 years 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 :) ).

@stephdau
2 years ago

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

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


2 years ago

#11 @iseulde
2 years ago

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

#12 @iseulde
2 years ago

I feel sorry for BuddyPress. :)

#13 follow-up: @paulwilde
2 years 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.

#14 @stephdau
2 years 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

#15 @stephdau
2 years 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 2 years ago by stephdau (previous) (diff)

#16 in reply to: ↑ 13 @iseulde
2 years 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.

#17 @stephdau
2 years 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 2 years ago by stephdau (previous) (diff)

#18 @stephdau
2 years ago

avryl: buddypress: worksforme

#19 @iseulde
2 years ago

@stephdau On mobile?

#20 @iseulde
2 years ago

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

#21 @stephdau
2 years ago

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

#22 @stephdau
2 years 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.

#23 @iseulde
2 years 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?

#24 @iseulde
2 years ago

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

#25 @stephdau
2 years 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 2 years ago by stephdau (previous) (diff)

#26 @iseulde
2 years 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.

#27 follow-up: @stephdau
2 years 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 2 years ago by stephdau (previous) (diff)

#28 in reply to: ↑ 27 @stephdau
2 years 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.

#29 @iseulde
2 years ago

Sounds good!

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

#30 @stephdau
2 years 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 2 years ago by stephdau (previous) (diff)

#31 follow-up: @iseulde
2 years ago

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

#32 in reply to: ↑ 31 @stephdau
2 years 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).

#33 follow-up: @iseulde
2 years ago

The donate button maybe?

#34 in reply to: ↑ 33 @stephdau
2 years 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 2 years ago by stephdau (previous) (diff)

#35 @stephdau
2 years 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. :)

#36 @stephdau
2 years 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).

#37 follow-up: @iseulde
2 years 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? :)

@stephdau
2 years ago

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

#38 in reply to: ↑ 37 @stephdau
2 years 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 2 years ago by stephdau (previous) (diff)

@stephdau
2 years 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).

@michalzuber
2 years 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

#39 @stephdau
2 years ago

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

@stephdau
2 years 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

#40 @stephdau
2 years ago

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

@stephdau
2 years 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.

@stephdau
2 years 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.

#41 @stephdau
2 years 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

@stephdau
2 years 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.

@stephdau
2 years 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.

@stephdau
2 years ago

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

#42 @stephdau
2 years ago

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

@stephdau
2 years 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

#43 @stephdau
2 years 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 2 years ago by stephdau (previous) (diff)

@stephdau
2 years ago

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

#44 @stephdau
2 years 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 2 years ago by stephdau (previous) (diff)

#45 @stephdau
2 years 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. :)

#46 @stephdau
2 years 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?

@stephdau
2 years 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.

#47 @stephdau
2 years 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

#48 @stephdau
2 years ago

  • Keywords has-patch added

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


2 years ago

#50 @helen
2 years 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.

#51 @helen
2 years ago

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

#52 @helen
2 years ago

In 29041:

Autoprefixer for [29040]. see #27440.

#53 @iseulde
2 years 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/

@michalzuber
2 years ago

Added braces and reuse the tab variable in if condition.

@iseulde
2 years ago

#54 @iseulde
2 years 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 ?>});.

#55 @iseulde
2 years ago

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

@michalzuber
2 years ago

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

#56 @stephdau
2 years 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. :)

#57 @michalzuber
2 years ago

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

#58 follow-ups: @ocean90
2 years 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.

#59 in reply to: ↑ 58 @ocean90
2 years ago

Replying to ocean90:

792px => 772px

#60 in reply to: ↑ 58 @stephdau
2 years ago

Replying to ocean90:

In 29125

Very cool. Nice work all.

#61 @melchoyce
2 years 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

@michalzuber
2 years ago

Added colors, props melchoyce

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


2 years ago

#63 follow-up: @Clorith
2 years ago

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

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


2 years ago

#65 in reply to: ↑ 63 @melchoyce
2 years ago

Replying to Clorith:

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

Related: #28883

#66 follow-up: @nacin
2 years 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.

#67 in reply to: ↑ 66 @celloexpressions
2 years 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).

@tellyworth
2 years ago

Incomplete - eliminate separate scrolling for content and fyi

#68 @ocean90
2 years ago

  • Keywords needs-patch added; has-patch removed

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


2 years ago

@stephdau
2 years ago

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

#70 @stephdau
2 years 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 2 years ago by stephdau (previous) (diff)

@stephdau
2 years 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

#71 @stephdau
2 years 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 2 years ago by stephdau (previous) (diff)

#72 follow-ups: @stephdau
2 years 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 2 years ago by stephdau (previous) (diff)

#73 in reply to: ↑ 72 ; follow-up: @stephdau
2 years 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 2 years ago by stephdau (previous) (diff)

#74 in reply to: ↑ 72 @stephdau
2 years ago

Replying to stephdau:

Long titles: see #28883

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

#75 in reply to: ↑ 73 @celloexpressions
2 years 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.

#76 @stephdau
2 years ago

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

#77 @stephdau
2 years ago

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

#78 @stephdau
2 years ago

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

@tellyworth
23 months ago

Scroll the entire thickbox content, header and all.

#79 follow-up: @tellyworth
23 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.

#80 in reply to: ↑ 79 ; follow-up: @stephdau
23 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 23 months ago by stephdau (previous) (diff)

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


23 months ago

#82 in reply to: ↑ 80 @tellyworth
23 months ago

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

#83 @helen
23 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.

#84 @helen
23 months ago

In 29480:

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

#85 @ocean90
23 months ago

In 29506:

Plugin details: Escape the donate link.

see #27440.

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


22 months ago

#87 @nacin
22 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].

#88 @nacin
22 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.