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

Review Perplex change for Segments #7659

Conversation

PerplexDaniel
Copy link
Contributor

@PerplexDaniel PerplexDaniel commented Feb 13, 2020

@nielslyngsoe
I ran into an exception when trying to add a new node when it was set to vary by segment only.
This latest commit should fix it. Server-side code would create 0 variants in this case, but it should always create 1 variant (the default / unsegmented variant).


This item has been added to our backlog AB#5083

@nielslyngsoe nielslyngsoe added the state/sprint-candidate We're trying to get this in a sprint at HQ in the next few weeks label Feb 13, 2020
@nielslyngsoe
Copy link
Member

The code looks good, whether this is the right approach for variants I need someone else to check. But I will merge this into our branch, so you have this available.

@azure-devops-sync azure-devops-sync bot changed the title V8/feature/ab4550 segments ui variant picker Review Perplex change for Segments Feb 13, 2020
@PerplexDaniel
Copy link
Contributor Author

Yeah actually the whole GetSegments() method is from my earlier server side code PR so this is just a small addition for the case of creating new nodes that apparently failed. I suppose this stuff isn't covered yet by unit tests otherwise this should have been caught already. But thanks in advance for merging this.

@nielslyngsoe nielslyngsoe merged commit 1a4e6e5 into umbraco:v8/feature/AB4550-segments-ui-variant-picker Feb 14, 2020
@umbrabot umbrabot removed the state/sprint-candidate We're trying to get this in a sprint at HQ in the next few weeks label Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants