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

Sitewide Notices - styling & functionality #137

Closed
hnla opened this Issue Apr 28, 2017 · 16 comments

Comments

3 participants
@hnla
Member

hnla commented Apr 28, 2017

Currently sitewide notices are in need of improving for markup & for functionality.

Issues:

Markup - needs better semantic rendering & classing ( currently updating to use <aside> rather than div & examining the use of a dl construct if we do output > 1 notice to users or regardless actually )

Not keen on strings meant to represent 'titles' or headings rendered using <strong> (true heading elements not appropriate for this type of construct)

Aria atts - need to review these and the requirements ( had the notion of adding role 'note' which is applicable for 'aside').

Functionality do we output lists if so we currently don't do only the last message added despite all messages possibly being 'live' and not dismissed by user. edit/ correction this is our BP behaviour to only activate the latest notice, checking in backend list of notices are active only for the last added. I would question this how do we know a user has seen all those messages, should we not list all until user dismisses ( could end up with too many though) unless the admin actually deactivates one for whatever reason, do we check user cookies for dismissed notices?)

We currently have no means of dismissing the notices despite alluding to a button function in templates?

Edit/ Just noticed that the notices seem to unset/dismiss themselves on page re-fresh, at least on return to page

@hnla hnla added the enhancement label Apr 28, 2017

@hnla

This comment has been minimized.

Member

hnla commented Apr 28, 2017

@dcavins Perhaps if you have time you could look at the functionality aspects outlined above?

  1. Messages seem to vanish for no real reason
    2, Message has no dismiss button
  2. Could we / should we think about looping all notices to screen for user to dismiss ( this might not be possible simply down to BP core even if we thought a good idea)

N.B:
Notices output in the user account header region.
Function is located in /bp-nouveau/includes/messagestemplate-tags.php
Original display reasoning was to not display as before as overlaid notice all screens but in user account screens and possibly as a admin bar bubble to draw attention to existence of.

@hnla hnla assigned dcavins and hnla Apr 28, 2017

hnla added a commit that referenced this issue Apr 28, 2017

Sitewide Notices revisions
Provisional updates to allow improved styling & presentation of notices.

* Update parent element to element `aside` rather than 'div'.
* Add classes for sitewide specific styling.
* Remove BR tag.
* Add `wpautop()` to message body to allow paragraph markup generation.

See #137

hnla added a commit that referenced this issue Apr 28, 2017

Provisional styles for sitewide notices
Simple first pass to define the sitewide notice elements, subject to change.

See #137
@dcavins

This comment has been minimized.

Collaborator

dcavins commented Jun 8, 2017

Some notes: The current logic for dismissing notices is that when they're displayed, they are immediately added to the closed notices usermeta: https://github.com/buddypress/next-template-packs/blob/master/bp-templates/bp-nouveau/includes/messages/template-tags.php#L74

Notices are displayed on the template_notices hook, at a very late priority, and only shown on the logged-in user's profile.

Is this behavior that we wish to change? Thanks!

@hnla

This comment has been minimized.

Member

hnla commented Jun 8, 2017

Need to look again but it doesn't feel correct or perhaps intuitive for user? There is the question of dismiss button functionality existing though and need to look at why that's not showing or whether it simply wasn't meant to in which case we need to remove mentions of them.

@hnla

This comment has been minimized.

Member

hnla commented Jun 9, 2017

@dcavins my problem is with the fact that we display the notice initially but instantly remove the message on any page re-fresh, so if I didn't spot, landed on wrong page, tutted clicked to another screen the message is removed and I have no option for seeing it again.

And if this is the proscribed behaviour what's the point of having button functionality to dismiss? I am wondering if I did something to affect behaviour early on in development!

Regardless I think the option for removing notices has to remain under user control?

@dcavins

This comment has been minimized.

Collaborator

dcavins commented Jun 9, 2017

OK, well let's just not auto-dismiss the notices, but instead require that the user manually dismiss them. Seems good to me.

dcavins added a commit to dcavins/next-template-packs that referenced this issue Jun 13, 2017

Change sitewide notice display.
Instead of using a one-off template tag specific to site-wide notices, push the site-wide notice into the `$bp->template_message` global. Then, the handling of the notices matches the handling of other user feedback and template messages.
• Add a "close" button instead of clearing notices as soon as they're shown.
• Add AJAX handler to update a user's `closed_notices` meta when the close button is clicked.
• Does not substantially change the display location of the messages.

See buddypress#137.

dcavins added a commit that referenced this issue Jun 13, 2017

Change sitewide notice display.
Instead of using a one-off template tag specific to site-wide notices, push the site-wide notice into the `$bp->template_message` global. Then, the handling of the notices matches the handling of other user feedback and template messages.
• Add a "close" button instead of clearing notices as soon as they're shown.
• Add AJAX handler to update a user's `closed_notices` meta when the close button is clicked.
• Does not substantially change the display location of the messages.

See #137.
@dcavins

This comment has been minimized.

Collaborator

dcavins commented Jun 13, 2017

The new commit changes the output like this:

Legacy used to create notices like this:

<div id="sitewide-notice" class="admin-bar-on">
	<div id="message" class="info notice" rel="n-1">
		<p>
			<strong>Attention</strong><br>
			Please be sure not to park in the white zone. The white zone is for loading and unloading only.
			<a href="#" id="close-notice" data-vivaldi-spatnav-clickable="1">Close</a>
			<input type="hidden" id="close-notice-nonce" name="close-notice-nonce" value="5d6082deac">
			<input type="hidden" name="_wp_http_referer" value="/members/two/">
		</p>
	</div>
</div>

Nouveau was building these items after the template notices were output:

<aside class="bp-sitewide-notice info" rel="n-1">
	<strong class="subject">Attention</strong>
	<p>Please be sure not to park in the white zone. The white zone is for loading and unloading only.</p>
</aside>

Nouveau, after this commit:

<div class="bp-feedback bp-messages bp-template-notice bp-sitewide-notice">

<p><strong>Seriously</strong><br>
			Don’t park in the white zone, OK?</p>


	<button type="button" title="close" data-bp-close="clear" data-vivaldi-spatnav-clickable="1"><span class="dashicons dashicons-dismiss" aria-hidden="true"></span></button>

</div>

The user must now manually dismiss the notice.

@hnla

This comment has been minimized.

Member

hnla commented Jun 14, 2017

One thing to comment on is the markup changes I did make specifically for sitewide notices i.e the change to the aside element to lend a little semantics to the mix, and generally improve the markup by removing the
& adding autop() into the mix, however can live with that partly as one of the major factors with Nouveau is the access to core functions at a template/theme level so devs can easily change.

Probably ought to sweep though JS just to check the rel attr described for the original parent elements rel=n-1 isn't used in some manner

I think we're still closing the message automagically when navigating to another page, but need to check this again, and still dithering as to whether this is as much a concern as I believe it to be.

@hnla hnla added this to In Progress in General Tasks Jun 14, 2017

@dcavins

This comment has been minimized.

Collaborator

dcavins commented Jun 14, 2017

@hnla Why not apply the improvements you made to the site wide notices to all of the notices? So let's add autop() as a filter here: https://github.com/buddypress/next-template-packs/blob/sitewide-notices/bp-templates/bp-nouveau/includes/template-tags.php#L192
and switch to an aside here: https://github.com/buddypress/next-template-packs/blob/sitewide-notices/bp-templates/bp-nouveau/buddypress/common/notices/template-notices.php

I'll add another commit that does these things to the branch.

@dcavins

This comment has been minimized.

Collaborator

dcavins commented Jun 14, 2017

@hnla

This comment has been minimized.

Member

hnla commented Jun 14, 2017

@hnla Why not apply the improvements you made to the site wide notices to all of the notices?

What a sensible solution - totally escaped me :) This is why it's helps so much having other eyes on things.

Yes lets update the markup as an <aside> I think it's an appropriate element choice for this sort of secondary page content.

N.B and ignore my mention of closing notices automagically, think that was a brain fart and had closed on click then forgotten :)

@hnla

This comment has been minimized.

Member

hnla commented Jun 14, 2017

@mercime Can I ask for a aria review of that markup I changed to in the first commit, I suggested we could add 'note' in the opening comment?

@mercime

This comment has been minimized.

Member

mercime commented Jun 20, 2017

@hnla You mean using aside instead of div? One thing is that when one activates the Sitewide Notices widget, you will have an aside inside of an aside - sidebar.

As an aside, no pun intended, there's an issue with the style of the sitewide notice
sitewide-message

Missing close link ^

Re: Adding "Note" - not sure if I got what you meant here. but I'm not partial to adding "Note" or any other text before the title of the subject nor message.

@mercime

This comment has been minimized.

Member

mercime commented Jun 20, 2017

Will be changing the empty link to close the message into a button with tooltip a la BP Trac #7421 in issue #78

mercime added a commit that referenced this issue Jun 20, 2017

Add button with bp-tooltip markup to close sitewide notice.
Per discussion in #137, the consensus is to have an element to
close the sitewide notice instead of the notice disappearing
when the user navigates to another screen. This commit adds the
button element along with the bp-tooltip markup as implemented
in https://buddypress.trac.wordpress.org/ticket/7421

See #78, #137, #163.
@hnla

This comment has been minimized.

Member

hnla commented Jun 20, 2017

Re: Adding "Note" - not sure if I got what you meant here

Meant as in aria role - sorry should have been clearer.

With SC like above and issues can you state always what browser, I'm guessing 'Chrome'?

Good point about the sitewide widget had forgotten all about that, despite having instigated it originally - memory, pah who needs one!

Yes it should be scoped to a parent content area , although we don't manage the markup of sidebars the theme does so the sidebar may not be described as a 'aside' always. For the moment we sideline 'aside'. Another frustration in trying to describe markup when one doesn't control the theme, starts to be tiresome! :(

@hnla

This comment has been minimized.

Member

hnla commented Jun 20, 2017

Ok notice display issue is with cover image disabled, another condition to check and style for :(

@hnla

This comment has been minimized.

Member

hnla commented Nov 14, 2017

Closing.

@hnla hnla closed this Nov 14, 2017

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