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

Theme update notices may be cut off #414

Open
nylen opened this issue Apr 15, 2019 · 7 comments

Comments

@nylen
Copy link
Member

commented Apr 15, 2019

From https://la-webeuse.com/classicpress-alternative-wordpress/:

The "New version available" notice for Twenty Fifteen, Sixteen, and Seventeen may be cut off in non-English languages.

This code was introduced in #385, and the related CSS (top: 39px) needs to be updated to allow the first notice to wrap to multiple lines if needed.

@bahiirwa

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

  1. I find the language above used hard to follow. Is the notice in yellow supposed to overlap the blue? Or the reverse?
  2. Does ClassicPress offer a translation for the the new message?

I am testing this by changing the language pack in general settings of CP Version 1.0.1+migration.20190313

Screen Shot 2019-06-18 at 16 16 16

@ginsterbusch

This comment has been minimized.

Copy link

commented Jun 18, 2019

my guess is, none of them should overlap each other, but the person who edited the original CSS in WP ignored (or was not even aware of) the fact that language strings may vary in length.

So in the original, the first message is supposed to be a short one, and the second one is a longer description of the above "header" message.

cu, w0lf.

@bahiirwa

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

I have tried fixing but since each language pack has its own height. However, having the z-index on .notice-warning makes things a little better. After the update, then everything goes back to default setting.

This is the change css into wp-admin/css/themes.css

.theme .notice-warning .notice-alt {
    z-index: 2;
}

.notice.update-message+.notice {
    top: 59px;
}

Screen Shot 2019-06-19 at 09 03 18

Screen Shot 2019-06-19 at 08 44 13

Having position: absolute and position: relative does not allow for having a dynamic height and positioning. Do we look to having flexbox come into play? That would solve the issue.

Can I use (caniuse.com) suggests Flexbox use:
Global: 94.44% + 3.87% = 98.3%
unprefixed: 94.15% + 2.18% = 96.33%

Screen Shot 2019-06-19 at 09 48 19

@nylen

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2019

We probably shouldn't introduce flexbox until we make a new major version that intentionally drops support for IE11.

It shouldn't be necessary here though. An alternative approach would be to put all the notices in a container element that has

position: absolute;
left: 0;
right: 0;
top: 0;
bottom: 0;

and then have the notices stack normally inside this element with default positioning and display: block.

@nylen

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2019

my guess is, none of them should overlap each other, but the person who edited the original CSS in WP ignored (or was not even aware of) the fact that language strings may vary in length.

And this is correct...and that person was me. 🤦‍♂

I know Germans especially like their words long - I think this was one of the last changes to get into v1.0.0 and I forgot to test in another language.

@nylen nylen added this to the v1.1.0 milestone Jun 26, 2019

@Norskes

This comment has been minimized.

Copy link

commented Jun 27, 2019

Wrapping is ok
image
Note that wrapper should be probably applied twise in themes.php (1 for usual php markup itself at str ~260 and 2 in a script template #tmpl-theme-single at str ~400).
But the real problem is not here. It's in mobile view:

image

Those containers are too small for the second notice anyway. And if I understand it right, height of containers is controlled by javascript, not CSS. I'll check JS bit later, but it seems to be.

Do we really need those notices on mobile? I'd offer to hide them. If they are critical, we can display them on :hover at full container height, overlaying the first notice, for example. Something like this (right box is ":hover"):

image

@nylen

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2019

Thanks @Norskes, I am assuming your wrapped screenshots are after making the change I suggested above.

On mobile we can't rely on :hover. I think we could hide the update notice, or another option would be to use a shorter, but less precise message:

Use "ClassicPress TwentySixteen" instead!

If we go this way we can echo both strings and use CSS breakpoints to show the correct one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.