Skip to:
Content

BuddyPress.org

#7732 closed defect (bug) (fixed)

BP-Nouveau: Group Creation Menu issues

Reported by: mercime Owned by: dcavins
Milestone: 3.0 Priority: normal
Severity: blocker Version:
Component: Templates Keywords:
Cc: dcavins

Description

  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/
  1. 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.
  1. 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.

Attachments (6)

group-create-navigation.png (16.7 KB) - added by mercime 18 months ago.
7732.1.diff (2.6 KB) - added by dcavins 18 months ago.
Fixes for bp_nouveau_group_creation_tabs().
create-a-group-nav.png (48.6 KB) - added by dcavins 18 months ago.
Result of changes.
7732.2.diff (14.5 KB) - added by dcavins 18 months ago.
Use core nav function, refactor styles.
7732.2-visual.png (21.3 KB) - added by dcavins 18 months ago.
Structure and style applied to core nav function output.
7732.3.diff (16.2 KB) - added by dcavins 17 months ago.
Update patch to remove now-unused function bp_nouveau_group_creation_tabs().

Download all attachments as: .zip

Change History (25)

#1 follow-up: @hnla
18 months ago

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.

#2 @dcavins
18 months ago

  • Cc dcavins added

#3 @dcavins
18 months ago

  • Owner set to dcavins
  • Status changed from new to accepted

@dcavins
18 months ago

Fixes for bp_nouveau_group_creation_tabs().

@dcavins
18 months ago

Result of changes.

#4 @dcavins
18 months ago

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

#5 @r-a-y
18 months ago

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.

Last edited 18 months ago by r-a-y (previous) (diff)

#6 in reply to: ↑ 1 @mercime
18 months ago

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.

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

  1. 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?

Last edited 18 months ago by mercime (previous) (diff)

#7 @DJPaul
18 months ago

  • Milestone changed from Awaiting Review to 3.0

#8 @DJPaul
18 months ago

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.

#9 @DJPaul
18 months ago

  • Severity changed from normal to blocker

@dcavins
18 months ago

Use core nav function, refactor styles.

#10 @dcavins
18 months ago

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
18 months ago

Structure and style applied to core nav function output.

#11 @DJPaul
18 months ago

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.

#12 @DJPaul
17 months ago

I am going to put this in later.

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


17 months ago

@dcavins
17 months ago

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

#14 @hnla
17 months ago

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.

#15 @DJPaul
17 months ago

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().

#16 @hnla
17 months ago

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.

#17 @hnla
17 months ago

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.

#18 @hnla
17 months ago

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

#19 @hnla
17 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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

Note: See TracTickets for help on using tickets.