-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Handle segmentation when segment alias includes underscore character(s) #19782
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a bug in segmentation handling when segment aliases contain underscore characters. The issue occurred in workspace editors where splitting variant folder parts on _
would incorrectly parse segments containing underscores, breaking the split-view editing functionality.
- Refactored variant parsing to use
indexOf('_')
instead ofsplit('_')
to handle underscores in segment names - Consolidated duplicate code by moving variant handling logic to the split view manager
- Updated preview app to show segment selector when any segments exist (previously required >1 segment)
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
member-workspace-editor.element.ts | Removed duplicate variant handling code, delegated to split view manager |
media-workspace-editor.element.ts | Removed duplicate variant handling code, delegated to split view manager |
document-workspace-editor.element.ts | Removed duplicate variant handling code, delegated to split view manager |
document-blueprint-workspace-editor.element.ts | Removed duplicate variant handling code, delegated to split view manager |
workspace-split-view-manager.controller.ts | Added centralized variant parsing methods and delimiter constant |
variant-id.class.ts | Fixed parsing logic to handle underscores in segment names correctly |
preview-segment.element.ts | Changed condition to show selector when segments exist rather than >1 segments |
Comments suppressed due to low confidence (1)
src/Umbraco.Web.UI.Client/src/packages/core/workspace/controllers/workspace-split-view-manager.controller.ts:21
- [nitpick] The constant name VARIANT_DELIMITER should follow TypeScript naming conventions and be in lowercase with underscores, or use a more descriptive name like VARIANT_PART_DELIMITER.
private readonly VARIANT_DELIMITER = '_&_';
Code updates looks good, someone will review and test soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Changed the base of this PR from main to release/16.2 to ensure it gets included.
- New ESLint rules have landed since the inception of this branch, so I updated a few things to allow the linter to pass 🎉
- Prettier wanted to chime in as well and correct a few syntactical things, such as dangling commas
LGTM
Ran into this one when working with segments in Engage - segmentation fails in the backoffice (ie split-view editing) when the segment alias includes underscore characters (which it does by default in Engage, and legacy uMarketingSuite installs, and by extension, in any segmented property data).
This is due to how string splits were handled in the various workspace elements when extracting culture and segment from the path:
See the issue? Splitting on
_
only works correctly whenfolderPart
contains a single_
. If the segment includes_
, we have incorrect segments:en-US_my-segment
=>en-US
,my-segment
💚en-US_my_segment
=>en-US
,my
🟥This PR changes the split handling and relocates some common code from different workspaces into the split view manager. There's a big chunk of duplication in these workspaces, around creating the split and single-view routes, but I've left that untouched (seems like an opportunity to add a
UmbContentDetailWorkspaceEditorBaseElement
or similar).Instead of splitting on
_
, we now find the first index of_
and take the corresponding sub-strings. I've moved the split handling into theUmbVariantId.FromString
function to ensure this can be reused easily.This does potentially change the culture in
handleVariantFolderPart
, where invariant cultures are now returned as null rather than 'invariant' - this is consistent with howUmbVariantId.FromString
has always worked, so I figured it's better to handle invariance consistently. When the resultantUmbVariantId
object is stringified, null cultures are set toinvariant
, so behaviour will not change from previous.Tangentially related, have also updated the preview app to show segment selector if any segments exist - previously this element only displayed if more than one segment, which meant documents with one segment could not switch to the segment from within the preview app (could preview from the backoffice only).