Problem/Motivation

There is some display options candidates for removal as they create more complexity and untested path than resolve real UX concerns.

As well as logic of grouping to challenge for UX benefits.

  • Library panels > Show library tabs as [label, icon, icon+label]
    • Remove: label only, keep flatten to discard tabs
  • View panels > Show panels as
    • Remove: label only
  • Toolbar buttons > State buttons [label, icon, icon+label, hidden]
    • Keep only: label
    • Hidden is a strange behavior as it hide the action but do not check access on API level, if we need to 'disable' an action it should be based on permissions, not UI tricks
    • Revert is a 'power user' option not very well explained, we should have an option to disable (Yes it contradict previous statement)
  • Toolbar buttons > Controls
    • Remove: hidden (same reason as above)
    • Unrelated actions (expand, help, theme): split?
  • Toolbar buttons > History [label, icon, icon+label, hidden]
  • Toolbar buttons > Back
    • Disable by default, evaluate later for value
  • Contextual panels > Show contextual tabs as [label, icon, icon+label]
    • Remove: label only

Issue fork display_builder-3616033

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mogtofu33 created an issue. See original summary.

mogtofu33’s picture

mogtofu33 commented
Issue summary: View changes
pdureau’s picture

pdureau commented

Hi,

My 2 cents.

✅ During #3576683: [meta] UX refresh, when I was promoting the move from a boolean to an Icon / Label / Icon & Label enumeration for library_tabs_display & contextual_tabs_display, it was a storage recommendation, for feature proofing, not an UX opinion.

So, if you want to restrict the available values at UI level, even switch to a checkbox sometimes to avoid a 2 items select list, it is fine, as long as we are still using the unified display_builder.panels_display (which can be used in display_builder.toolbar.button too) storage side.

Toolbar buttons > State buttons [label, icon, icon+label, hidden]. Hidden is a strange behavior as it hide the action but do not check access on API level, if we need to 'disable' an action it should be based on permissions, not UI tricks

✅ I totally agree, it was the topic of this ticket #3548769: Security: availability of endpoints according to islands which may be closed if we remove hidden here.

Toolbar buttons > Controls. Remove: hidden (same reason as above)

⚠️ ControlsButtons is a different case. Each button is its own specific feature, we have grouped them for better management: #3545608: Merge some buttons into a Controls island. So removing hidden is enforcing the feature to the users and it is better to be careful. I guess not everybody wants expand (by the way, still making sense since #3542003: Responsive viewports with iframes which is always displaying the Display Builder UI Full page and is moving the chrome to the preview panel), theme and help displayed for every profiles.

Toolbar buttons > Back. Disable by default, evaluate later for value

⚠️ Do we have another way of leaving the Display Builder UI? If not, why not keeping it for now? Is it not something people always need?

mogtofu33’s picture

mogtofu33 commented
Issue summary: View changes
mogtofu33’s picture

mogtofu33 commented

I want to remove the option completely so we have only label.

Imho on UX side it makes sense only for library panels because of the wrapping tabs and sidebar default width (17rem) which wrap the tabs.
I am ok to remove it here too for sake of simplicity, so we need to change left sidebar default width to ~20rem.

For ControlsButtons that's why I offer split to fix this issue and have hidden obsolete.
So theme switch can be disable by default, expand and help are enabled by default.

Do we have another way of leaving the Display Builder UI? If not, why not keeping it for now? Is it not something people always need?

Yes the admin navigation/toolbar, so candidate for removal but if expand is on it need 2 clicks...

The first intent of 'back' was to answer:

How do I simply change options and profile of current display

Could be pages conditions and for others only profile. Entity override goes to front (? need a fix it seems EntityViewOverride::getDisplayUrlFromInstanceId()).

But it is clearly too convoluted, not clear on UX, and Instances list should be the place with a possible link to display settings.

After more thought seems we can keep back.

mogtofu33’s picture

mogtofu33 commented
Assigned: mogtofu33 » pdureau
Status: Active » Needs review

In review for feedback and possible warning before doing the work.

pdureau’s picture

pdureau commented
Assigned: pdureau » mogtofu33

I want to remove the option completely so we have only label.

OK that's great, it will simplify logic and admin UI.

For ControlsButtons that's why I offer split to fix this issue and have hidden obsolete.
So theme switch can be disable by default, expand and help are enabled by default.

OK

Entity override goes to front (? need a fix it seems EntityViewOverride::getDisplayUrlFromInstanceId()).

Indeed, it like that since day one:

  public static function getDisplayUrlFromInstanceId(string $instance_id): Url {
    return Url::fromRoute('<front>');
  }

Maybe it worth adding tests for DisplayBuildableInterface::getDisplayUrlFromInstanceId()

After more thought seems we can keep back.

OK

mogtofu33’s picture

mogtofu33 commented
Status: Needs review » Needs work
Related issues: +#3616313: Check static methods on DisplayBuildableInterface

Note, the EntityViewOverride::getDisplayUrlFromInstanceId()) issue is cited in #3616313: Check static methods on DisplayBuildableInterface but could be done in this issue or the other.

mogtofu33 opened merge request !348

mogtofu33’s picture

mogtofu33 commented
Assigned: mogtofu33 » pdureau
Status: Needs work » Needs review
pdureau’s picture

pdureau commented
Assigned: pdureau » mogtofu33
Status: Needs review » Reviewed & tested by the community

RTBC:

  • Code review: OK
  • Feature review (without fresh install, after a drush updb): OK
  • Feature review (without fresh install, after a drush updb, saving an unmodified profile): still OK
  • Feature review (fresh install): OK

Also, I am not sure we have finished the talk about expand button in #3542003: Responsive viewports with iframes. I am sorry if the subject has already found its conclusion (I don't remember, to be honest...) , but do we still need the "Expand" button in Controls island now the Display Builder is always displayed "full page" (with the toolbar always at the top and the "chrome" moved to inside the Preview panel)?

mogtofu33’s picture

mogtofu33 commented
Assigned: mogtofu33 » Unassigned
Status: Reviewed & tested by the community » Fixed

With navigation left and top bar expand is useful.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • mogtofu33 committed 8c7ef175 on 1.0.x 
    task: #3616033 Reduce ProfileViewBuilder options

By: mogtofu33
By:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.