Sitewide Notices - styling & functionality

[hnla]

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]

@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.

[dcavins]

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]

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]

@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]

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

[dcavins]

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]

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.

[dcavins]

@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]

It appears that BP already adds many filters: https://github.com/buddypress/BuddyPress/blob/master/src/bp-core/bp-core-filters.php#L48

[hnla]

@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]

@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]

@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

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]

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

[hnla]

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]

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

[hnla]

Closing.