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

Fixed theme update notices (overlapping, fixed height and excessive length) #431

Open
wants to merge 3 commits into
base: develop
from

Conversation

@Norskes
Copy link

commented Jun 29, 2019

Description

  1. Wrapped notices with a common "div.notices". Notices now have responsive height and no top margin. External positioning is provided by a wrapper. (According to discussion: #414 (comment))
  2. Wrapped secondary parts of related long messages to hide them on mobile (native breakpoint is 1120px).
  3. Added __() to some strings that had no internalization.

Motivation and context

Issue, help wanted: #414
Proposal: #414 (comment)

How has this been tested?

Visually tested changes in Chrome and Firefox. First at full witdh then decreasing viewport to activate breakpoint. Tested in English and Russian locales + adding hypothetical additional length

Screenshots (if appropriate):

Desktop, FF:
image
Mobile, Chrome:
image
Mobile, FF:
image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Norskes added some commits Jun 27, 2019

wrapped notices to provide adaptive height
But the mobile view is still not good
Cut wrappers + i18n for some strings
Wrapped secondary parts of long messages and hide them on mobile to fit parent container size.

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

@nylen

This comment has been minimized.

Copy link
Member

commented Jun 29, 2019

Thanks for the PR, the direction here looks good.

There is one more place that probably needs to be updated - these notices can also appear in the Customizer: https://github.com/ClassicPress/ClassicPress/pull/385/files#diff-ad7d9b797ea9279bf01bf89b72af6d8fR101

Needs detailed review & testing but I think it is pretty much there.

Updated theme notices markup structure in customizer
Added a common notices wrapper  + __() around message string
@Norskes

This comment has been minimized.

Copy link
Author

commented on acbbd73 Jun 29, 2019

Chrome:
image

FF:
image

Chrome ("mobile"):
image

FF ("mobile"):
image

@Remzi1993
Copy link
Contributor

left a comment

Looks fine to me 😉

@nylen

This comment has been minimized.

Copy link
Member

commented Jul 14, 2019

@Remzi1993 : What testing were you able to do here?

@Remzi1993

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

@nylen I did not run tests specifically, just installed this branch with PHP display_errors = on
I think this is not enough testing, so I let you decide.

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