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

[3.x] [ReUp] Changed status message appearance and position #14003

Merged
merged 4 commits into from Aug 13, 2018

Conversation

Projects
None yet
5 participants
@jonleverrier
Contributor

jonleverrier commented Jul 19, 2018

What does it do?

This PR changes the default appearance and position of the status notification (the one you receive after saving a resource).

modx3_message_save2

screen shot 2018-07-19 at 14 32 47

Based on feedback from Slack, this PR also adds some css utility styles so you can position the status message in different positions on the screen. These are .modx-status-msg.has-position-center-center, .modx-status-msg.has-position-center-top, and .modx-status-msg.has-position-right-top.

If one day, somebody decides to create a system setting for the position, they can hook into these css utility classes.

Screen shot of .modx-status-msg.has-position-right-top
screen shot 2018-07-19 at 14 26 41
Screen shot of .modx-status-msg.has-position-center-top
screen shot 2018-07-19 at 14 26 16
Screen shot of .modx-status-msg.has-position-center-center
screen shot 2018-07-19 at 14 25 35

Why is it needed?

In 2.x the status message was almost invisible. In MODX 3 we have lots of new styles - styling up the status message makes the product feel more consistent.

It also moves the status message away from the action buttons at the top, which are unusable whilst the message is in display.

Related issue(s)/PR(s)

Mentioned in #13988, #13942

changed appearance and default position of .modx-status-msg. also add…
…ed some utility class names to display the message in different positions

@jonleverrier jonleverrier requested review from Mark-H and opengeek as code owners Jul 19, 2018

@JoshuaLuckers

It might be a good idea to define the variables in _colors-and-vars.scss like $modx-status-msg-bg: $white.

It’s makes it easier to customize if those variables are in one file.

@jonleverrier

This comment has been minimized.

Contributor

jonleverrier commented Jul 19, 2018

@JoshuaLuckers The background colour is being defined from the default success colour, which i think is $green. You're saying it's worth creating a variable just for the status message background colour?

@JoshuaLuckers

This comment has been minimized.

Contributor

JoshuaLuckers commented Jul 19, 2018

All color variables should be defined in _colors-and-vars.scss. The background color was just a little example.

@jonleverrier

This comment has been minimized.

Contributor

jonleverrier commented Jul 19, 2018

Understood. The status message colour (text, icon, background) is using variables from _colors-and-vars.scss. If you think I should create new variables specific to the status message, let me know and i'll get this changed!

@JoshuaLuckers

This comment has been minimized.

Contributor

JoshuaLuckers commented Jul 20, 2018

I do not know what the core team prefers. It's my personal preference to have variables for specific components to be in a "variables" file.
For example, I want to the text to be black and no border radius for this specific component. I now have to modify the "core" component file and increase the risk of my custom modifications to be overwritten when updating MODX. If those specific variables where in the "variables" file I could create a custom variables file "overruling" the "core" variables and thus reducing the risk of my changes being overwritten.

TLTR: It makes it easier to theme core components.

@jonleverrier

This comment has been minimized.

Contributor

jonleverrier commented Jul 29, 2018

@JoshuaLuckers added specific sass variables for the status message

@jonleverrier

This comment has been minimized.

Contributor

jonleverrier commented Aug 10, 2018

@Mark-H @Alroniks - is this likely to get merged into 3.x or does it require more discussion? Is there anything more I can do?

@OptimusCrime

This comment has been minimized.

Contributor

OptimusCrime commented Aug 10, 2018

This looks fine by me.

@Alroniks

This comment has been minimized.

Collaborator

Alroniks commented Aug 10, 2018

I like this solution but have not check yet how it works, so cannot approve right now. But will try today.

position: fixed;
top: 10px;
right: 15px;
width: 25%;

This comment has been minimized.

@Alroniks

Alroniks Aug 13, 2018

Collaborator

It seems 25% too much. On wide screens, it looks strange. I guess it can be solved by adding max-width property.

This comment has been minimized.

@jonleverrier

jonleverrier Aug 13, 2018

Contributor

happy to add a max-width to the message. I guess the original thinking behind the 25% width, is that is takes up 1 quarter of your screen, regardless of how wide.

This comment has been minimized.

@Alroniks

Alroniks Aug 13, 2018

Collaborator

Yes, of course. But on too wide screens it looks not so awesome as expected.

@jonleverrier

This comment has been minimized.

Contributor

jonleverrier commented Aug 13, 2018

should be good to go now @Alroniks @Mark-H !

@Mark-H

Mark-H approved these changes Aug 13, 2018

@Alroniks Alroniks self-assigned this Aug 13, 2018

@Alroniks Alroniks merged commit 34390ad into modxcms:3.x Aug 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Alroniks added a commit that referenced this pull request Aug 13, 2018

Changed appearance and position of status message after saving action [
…#14003]

* upstream/pr/14003:
  added reset for max-width on mobile
  added max-width property to support manager users with wider screens
  added sass variables for status message
  changed appearance and default position of .modx-status-msg. also added some utility class names to display the message in different positions

@jonleverrier jonleverrier deleted the jonleverrier:successmessage branch Aug 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment