BP-Nouveau: Group Creation Menu issues

[mercime]

1. Links in the group create menu, representing the current or completed steps, lead to 404 page. This is because there's an additional http:// generated between the groups slug and the step like so ​http://localhost/codex/groups/http://create/step/group-details/

2. The group create menu contains links which are not links at all, e.g. <a disabled="disabled">5. Invites</a>, representing step/s to be completed in the group creation process.

3. Periods are missing after #s 1 to 3 in the menu.

Enhancement if there's time: replace the UL wrapper for the menu with an ordered list and style it like a progress bar.

Attaching screenshot.

[hnla]

How have these links gone wonky last time I checked create step they were ok have we changed something??

As for enhancements we best focus on other areas first, these steps are desperately in need of a re-write altogether which might have made updating styling worthwhile but for the momentlets back burner.

[dcavins]

Fixes for bp_nouveau_group_creation_tabs().

[dcavins]

Result of changes.

[dcavins]

I've added a patch that addresses these issues:

1. Fix links in create menu. Problem here was an echo function used inside of an echo statement.

2. The group create menu contains links which are not links at all, e.g. <a disabled="disabled">5. Invites</a>. Replaced with the link to the farthest accessible step. I'm not sure this is great. It's a real link, and doesn't take you somewhere you're not supposed to go yet, but it seems a bit off.

3. Periods are missing after #s 1 to 3 in the menu. Fixed. These periods are hardcoded in the output, which could be a problem for sites not in English, maybe? I'm not sure about that.

See attached patch and image showing the changes.

[r-a-y]

Just took a quick look at the patch and I'm wondering why we have a separate template tag in Nouveau solely for the group creation tabs.

Is there something wrong with the existing bp_group_creation_tabs() function?
​https://buddypress.trac.wordpress.org/browser/tags/2.9.3/src/bp-groups/bp-groups-template.php#L4647

If so, we should make that function better instead of forking that code for Nouveau.

[mercime]

Replying to hnla:

How have these links gone wonky last time I checked create step they were ok have we changed something??

@hnla Noticed those menu links only by chance last night when I tapped the screen which went to the 404 page.

As for enhancements we best focus on other areas first, these steps are desperately in need of a re-write altogether which might have made updating styling worthwhile but for the momentlets back burner.

Perhaps a minor change from UL to semantic OL if there's time? :D

Replying to David Cavins:

1. Fix links in create menu. Problem here was an echo function used inside of an echo statement.

Thank you.

2. The group create menu contains links which are not links at all, e.g. <a disabled="disabled">5. Invites</a>. Replaced with the link to the farthest accessible step. I'm not sure this is great. It's a real link, and doesn't take you somewhere you're not supposed to go yet, but it seems a bit off.

@dcavins, you're right that a link that doesn't take you somewhere is off. Sorry, my post above was not complete. The disabled attribute can only be applied to the following HTML elements: <button>, <input>, <select>, <textarea>, <optgroup>, <option>, and <fieldset>.
References:
​W3C.org
​Mozilla Developer (the keygen and command elements listed in Mozilla are deprecated)
​Read why this proposal to allow using the disabled attribute for a link is not going to happen

One solution could be just removing the surrounding <a> from the group menu list item that a user has not gone through yet.

3. Periods are missing after #s 1 to 3 in the menu. Fixed. These periods are hardcoded in the output, which could be a problem for sites not in English, maybe? I'm not sure about that.

@dcavins ​number_format_i18n?

P.S. "Edit your group's name & description" is good for e.g. esp.com/groups/<group-name>/admin/edit-details/ but I don't think it fits the first step of the Group creation process. Perhaps just "Your group's name & description" there?

[DJPaul]

The group create menu contains links which are not links at all, e.g. <a disabled="disabled">5. Invites</a>. Replaced with the link to the farthest accessible step

I think this doesn't make sense. If we don't have a link, then it should not be a link.

Is there something wrong with the existing bp_group_creation_tabs() function?

@r-a-y There is a lot of this in the Nouveau code base. Many of them exist because they duplicate functions contained within the Legacy files, but not all. I suspect it's going to stay like this unless a contributor has a bunch of time to audit a hundred functions or so.

[dcavins]

Use core nav function, refactor styles.

[dcavins]

I've just attached a patch that goes the other way-- It uses the core function as suggested by @r-a-y (a good idea I think) and refactors the CSS to handle the slightly modified markup. This also necessitates a change, adding a "count" class to count spans in nav items, which I'm also in favor of (so that "count" styling is applied to .count, not any old span). Comments welcome!

[dcavins]

Structure and style applied to core nav function output.

[DJPaul]

We can remove bp_nouveau_group_creation_tabs() with 2.patch. This is fine with me but let's ask @hnla to sign-off on the change.

[DJPaul]

I am going to put this in later.

[dcavins]

Update patch to remove now-unused function bp_nouveau_group_creation_tabs().

[hnla]

Why are we removing Nouveau functions :( Folks half the point in many of Nouveaus approaches to things like core functions was to explicitly attempt to pull the more front end rendering functions out of core and to a position where they could be easily manipulated, so on face value it may seem that we are just duplicating code but there was a reason ( albeit possibly not in this case but I would need to review as it's been a while since I last dealt with this steps aspect)

Can we please be careful not to rush to undo the work imath and I did for Nouveau unless there a definite issue identified.

@mercime While the UL is not an unsemantic in itself I agree moving to an OL might be better for 'steps', think we can do that, had a quick check and we do have to update rules for tabbed links as a comp as they're based on 'UL' so would need to double up or probably better remove UL selector and ensure only class manages the tabs effect. I can look at that when the rest of this ticket is dealt with.

[DJPaul]

bp_nouveau_group_creation_tabs() is an identical duplicate of bp_group_creation_tabs() which is a template function meant to be used by these parts. I suspect there are others like it. There's no problem with bundled template packs duplicating functions out of BP-Legacy, but not functions that live in BuddyPress core -- unless there's a good reason. There isn't for bp_nouveau_group_creation_tabs().

[hnla]

Not disagreeing it's an identical function but as above it's more about re-positioning core template functions to a template level where they can be easily managed/overloaded.

I'm not going to argue over this though I haven't the time or energy :)

my opinion is to use @dcavins two patches, possibly refactor the li elements children so we don't generate the links on non link/active steps, change the UL to OL and update the styles to account for removed anchors & OL element numbering, If we go that route I'll handle the OL change and the ruleset updates.

Or we dump the function, when we have a consensus & commit approach I'll step in and handle the OL change and refactoring the rules to be less tied to elements and hook on the tab class.

[hnla]

If we go with @dcavins 2nd patch we'll need to remove the actual bp_nouveau_group_creation_tabs() as well no point having redundant functions hanging around.

@DJPaul Make an executive decision on which way to go and I'll effect the changes later so we have a 'blocker' removed.

[hnla]

In 11973:

Nouveau: Update group_creation_steps function

Commit addresses markup issues in Nouveau function.

Revert group create steps nav function to use BP core function.

This removes the problem of nested echo function & anchor links being set when they are not an active link.

Commit removes redundant Nouveau custom template tag function.

Commit adds class 'count' to spans generating numbers e.g steps count or group/member counts in navs.

Props dcavins, mercime, djpaul, r-a-y

See #7732

[hnla]

In 11974:

Nouveau: Update group creation links to OL element

Commit changes list construct for create step links to use an ordered list rather than unordered in groups/create.php.

Adjusts styles to reflect use of either ul/ol in tabbed links rulesets.

Props mercime, hnla

Closes #7732