search

WPTT / WPThemeReview

PHP_CodeSniffer rules (sniffs) to enforce WordPress theme review coding conventions
MIT License
207 stars 38 forks source link

Allow `add_menu_page` & `add_submenu_page` #254

Open aristath opened 4 years ago

aristath commented 4 years ago

This was discussed and decided in yesterday's meeting. Checks for add_menu_page and add_submenu_page should now be removed.

joyously commented 4 years ago

It was mentioned, it was decided. No discussion really about the consequences happened.

aristath commented 4 years ago

@joyously that's why yesterday we had a 90-minute meeting, where we discussed things. There were no objections from anyone, so it was decided to move forward with this.

If you have any objections by all means feel free to say what they are. Is your objection regarding the decision itself, or the decision process?

justintadlock commented 4 years ago

I was unable to attend yesterday's meeting, so I'll formally raise my objection here.

What is the reason to break away from the standard Appearance menu and change the user experience? How many themes would truly benefit from having a top-level menu (in terms of making a better user experience)? What issues may arise from allowing top-level menus for all themes?

I think it'd be good to discuss those things before pulling the trigger on this one.

joyously commented 4 years ago

Both. There was not much attendance at the meeting. There were no pro and con lists for either side. There is no reason to make the change.

acosmin commented 4 years ago

  1. One reason for allowing it is that the Customizer will be used less, and there are concerns that authors will try to move their upsell to the editor itself (as mentioned by poena)

  2. Another reason would be that plugins can do it, why not themes. You can only use one theme, that's just one menu item.

  3. It is more visible.

With strict requirements it can be a good thing. I don't see any cons.

pattonwebz commented 4 years ago

I don't have any objections to users having a top-level item providing that it is a standard item, with no priority modifications and no custom styling.

In my oppinion it neither improves or degrades user experience to have it a top level item vs having it under appearance. What could become an issue is the text users choose as their link title though or decide they want to add a fancy styled icon or do something crazy with the link colors.

I am stronly in favor of the title being short enough to fit onto only a single line and matching the theme name however some theme names are very long and would wrap. A check could be added here to enforce a max length of the title.

Traditionally users have had only the customizer as the only persistant place they can link to their page but use of customizer is dwindling due to the forthcoming FSE changes and also the ability to inline widgets through the editor now instead of going to customizer to add those.

What is incresingly common is users using their activation notice to display information better placed on their custom page and team reps having to constantly contact those authors and request they change it. Allowing this more prominant location for a link could help to reduce that happening so often.

pattonwebz commented 4 years ago

Both. There was not much attendance at the meeting. There were no pro and con lists for either side. There is no reason to make the change.

I can't make time for people to attend the meeting but that shouldn't stand in the way of trying new things if those who were active thought it was worth trying. I think this is worth trying - or at the very least opening disucssions here about it. Everyone is welcome to pop by the channel and chat about it too :)

aristath commented 4 years ago

The concern is that we limit things too much, and as a result authors will keep finding ways to circumvent the checks. They were not allowed to add a top-level admin menu, so then they started adding admin footer links. Then it was dashboard widgets. Those were disallowed, so they started adding extremely annoying admin notices. Then a rule was added to force admin notices to a minimum, make them dismissable, only follow core styles etc. Then there was the customizer issue with upsells, so more rules had to come for that. The more we restrict things, the more we force authors to find new, creative - and annoying ways to get around the rules. We understand that themes are businesses and they need to upsell, or even genuinely have a visible place where they can add documentation for example. With customizer losing its importance in a post-FSE WP, theme-authors will start searching for new places to put their upsells & info. They can add it in the paragraph block settings, a big notice there saying "upgrade to PRO for x/y/z features". Nobody wants that... And it's impossible to check all block settings for example, or metaboxes etc. We can't stop all things... and these things keep coming up. Allowing themes to add a top-level admin menu will give them the visibility they need, without polluting all other areas of the dashboard. It's a way to prevent future abuse in areas like the editor, a valve to let off steam before it explodes in the wrong directions.

jrfnl commented 4 years ago

Just out of interest: why allow a toplevel menu ? As there is a Theme menu already, wouldn't it make more sense to only allow the Theme to add submenu pages and only allow them within the Theme menu ?

Note: I have no stake in this discussion, it is just a question which popped into my head when I read through the comments here.