Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Collection: Sets label and icon of Workspace View #2113

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

leekelleher
Copy link
Member

Description

Attempts to fix:

When setting a custom label and icon for a Collection (data-type), the workspace view's label and icon were not being updated.

This PR adds a UmbWorkspaceEditorContext, which moves logic from the UmbWorkspaceEditorElement to collect the manifests and build the routes & views.

The context exposes a method .setWorkspaceViewMeta() to enable a workspace view's label and icon be configured, (based on the workspace view's extension alias).

Types of changes

  • New feature (non-breaking change which adds functionality)

How to test?

  1. Configure a document-type to use a collection, (Structure > Presentation > Collections)
  2. In the corresponding data-type for the Collection, set the "Content app icon" and "Content app name" fields
  3. Go to the document (either create new or existing). Has the icon and label changed?

Notes

  1. There is an edge-case, where a server-side migration (for v14) sets the default icon for a Collection to be icon-badge color-black, but that particular icon doesn't exist in the backoffice for v14. So I have added a check for this, and set it to icon-grid.

  2. There is an annoying UI glitch, where the default icon and label (from the Collection manifest) is displayed, before changing to the custom icon and label. (Sometimes it happens faster, other times slow, try setting the video playback speed to 0.25x).

Recording.2024-07-11.171936.mp4

This is because the Workspace Editor loads in the Workspace View manifests first, then it can calculate the routes, then the Workspace View Collection is loaded in (from that route), which then has access to the data-type configuration and sets the custom icon and label.

  1. I have commented out code to handle the showContentFirst, but I hit various issues that I'm not currently able to resolve. e.g. moving the "Content" workspace to the first position, but the route doesn't update, so it displays the "Collection" instead. There is also a similar UI glitch with the Workspace View tabs updating, (it shows "Collection" first, then suddenly swaps with "Content").

I'll mark this PR as a draft until we've addressed the above issues.

Copy link

sonarcloud bot commented Jul 25, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint


// TODO: Review how `showContentFirst` is implemented, and how to re-route/redirect the path.
// e.g. the "Content" tab shows the "Collection" view. [LK]
if (showContentFirst) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the only right way to do this, would be to manipulate the weight of the Collection based on this setting. (
Maybe assuming that content has a certain weight?)

Not completely sure how at the moment of writing this, but if feels wrong to sort the views, also notice how they can come and go depending on conditions. Atleast of now, then it should use the weight to sort and then make sure to sort(& change weight based on setting) in the #createViews method.

import type { UmbRoute, UmbRouterSlotInitEvent, UmbRouterSlotChangeEvent } from '@umbraco-cms/backoffice/router';

const elementName = 'umb-workspace-editor';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a optimization we want to implement in general, I think it is a bit of an overload and I must admit i'm in for consistency. And using a variable twice use is not directly enough to trigger my need for this, especially because the later one is only used by TypeScript, so basically we are crating a const to use it once.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There have been many cases where there has been a mismatch between the registered element name and the type because we have renamed elements over time. I think this is a reasonable way to avoid that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants