Flash message at top of the page not visible

[hemahg]

What this PR does / why we need it:
Flash message at top of the page not visible when the page is large

Which issue(s) this PR fixes
https://issues.redhat.com/browse/THREESCALE-4028

Verification steps

• Go to API > Applications > Application Plans(http://provider-admin.example.com.lvh.me:3000/apiconfig/services/2/application_plans)
  -Create an application plan and observe the flash message

[cntlsn]

@damianpm I would suggest to only add some styling for now.

#flashWrapper could be:

#flashWrapper {
    display: block;
    position: fixed;
    background-color: #fff;
    padding: 1rem;
    margin-top: 1.5rem;
    -webkit-box-shadow: 0px 0px 20px 0px rgba(0,0,0,0.3);
    -moz-box-shadow: 0px 0px 20px 0px rgba(0,0,0,0.3);
    box-shadow: 0px 0px 20px 0px rgba(0,0,0,0.3);
}

And I would make the text content live into a span instead of a p (in order to lose p styling and avoid additional rewrites):

<span class="flash-message flash-message--notice">Some text</span>

What do you think?

[damianpm]

@cntlsn @hemahg @thomasmaas
OK, but I'd keep the alignment to the right and the animation

#flashWrapper {
  @include animation(fadeInLeft 0.5s ease-in-out);
  background-color: #fff;
  box-shadow: 0px 0px 20px 0px rgba(0,0,0,0.3);
  display: block;
  margin-top: 1.5rem;
  padding: 1rem;
  position: fixed;
  right: 0;
}

[cntlsn]

@damianpm @hemahg absolutely! I forgot to say more explicitly that my idea was to add those properties on top of the existing ones!

[josemigallas]

@damianpm @cntlsn
For what is worth I think it looks better with a padding-right margin-right, since it's a floating element with the shadow and all.

@hemahg
I would also use this opportunity to apply conditional rendering. This means you'd check the condition before rendering instead of hiding it with display: none.

= render 'shared/provider/flash', flash: flash unless flash.empty?

[thomasmaas]

I think the display: none; construct is to make the the flash message available to javascript (<div class="ajaxNoticeValid">. Could be that we no longer use that though.

Styling-wise I kind of like the error message sticking to the right but would be good to deploy to preview.

[cntlsn]

@josemigallas do you mean margin right?
Just to be clear, if my styling is added to the current style of #flashWrapper, as suggested in a previous comment, it should look like this:

cc @damianpm @hemahg

[damianpm]

@josemigallas do you mean margin right?
Just to be clear, if my styling is added to the current style of #flashWrapper, as suggested in a previous comment, it should look like this:

cc @damianpm @hemahg

I prefer it aligned to the right

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@44c883a). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #1629   +/-   ##
=========================================
  Coverage          ?   80.23%           
=========================================
  Files             ?     2245           
  Lines             ?    70376           
  Branches          ?        0           
=========================================
  Hits              ?    56465           
  Misses            ?    13911           
  Partials          ?        0

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44c883a...5000052. Read the comment docs.

[damianpm]

@thomasmaas @cntlsn
It's deployed in preview, if you want to check
https://multitenant-admin.preview01.3scale.net/p/admin

[cntlsn]

@damianpm @hemahg
It looks like the text content is still wrapped in a p tag, which breaks the padding:

[cntlsn]

@hemahg @damianpm
Tested in https://multitenant-admin.preview01.3scale.net/, both css formatting and z-index issues look good to me.

[thomasmaas]

Would be good if the margins and paddings (and box shadow) would be same as rest page. This means you have to use the `line-height-times' function you will see in other places.

[Martouta]

@damianpm @hemahg I remove myself from the reviewers here because I do not think that I am the ideal person to review this 😄

[hemahg]

@thomasmaas, @cntlsn Updated margins and paddings with `line-height-times'

[cntlsn]

@hemahg @damianpm @thomasmaas as I shared in chat already, my feeling is that a more pronounced drop shadow (like the one I posted previously) will make the alert stand out more from the background. This is good UX practice, as it makes the alert more noticeable to users.

[thomasmaas]

@cntlsn yes, I already told @hemahg earlier, it's already fixed in code.

[thomasmaas]

@hemahg I would suggest using half line-height margins like so:

correction: that doesn't work well when there is a scrollbar. So maybe 1 lineheight is better:

That makes it align with page content but I'd say that animation + drop shadow size still make it notable enough.

[thomasmaas]

Suggested change

	right: line-height-times(2);
	right: line-height-times(1);

[thomasmaas]

Found a page with an item that also has z-index 1000. You want to serch css for z-indexes and make sure you sit above all.

[thomasmaas]

Good job @hemahg 💪🏽