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

V8/feature/ab4550 segments ui variant picker #7676

Conversation

PerplexDaniel
Copy link
Contributor

@PerplexDaniel PerplexDaniel commented Feb 17, 2020

Hi @nielslyngsoe,

I added some additional commits to fix a few things. The C# changes will probably have to reviewed by someone else at some point, but perhaps that is better to do when we feel it's good enough and can be checked. I now simply fix stuff I run into and sometimes it has to be changed again later (like the GetSegments() method).

New changes now:

  • GetSegments() updated (again) to always return the default segment (null). I ran into a case where there was no data yet for the default segment but there was data for other segments and in that case the default segment was not returned from the GetSegments() method, meaning in the Umbraco frontend you could no longer edit the default segment at all.
  • Null reference exception fix related to the assumption a culture would be present
  • Small optimization in the requestSplitView function to prevent looping through all variants after variant has been found
  • Experimental fix to prevent segments to be deleted each save when their value is null.
    • I noticed whenever a Save or Publish is performed Umbraco does something like "delete all property data of version X" followed by "insert all relevant property data of version X", and decides that property data with a null value is not relevant so is skipped. However, for segments this means the whole segment is removed because we only have the property data to know which segments exists. So when I want to create a new empty segment I just write an empty value to some property but Umbraco was removing old segments when saving a new one (i.e., if I create segment A, then B, then C all with null or "" values, only segment C would be present after the last save/publish).

This item has been added to our backlog AB#5131

…iants and/or somehow open multiple.

It is not possible to break out of _.each using a return statement, it simply returns that current function
but _.each will continue calling the others after that.
During save/publish, Umbraco first deletes all property data of a specific version
and then adds property values again. However, any segments that were created but had
an empty value would not be added again which meant the segments were entirely gone afterwards.
…when there are no segments at all.

This makes sure that even if there is no property data for the default segment in the database but only for
some segments, the default segment will still be returned here.
@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 17, 2020
@nielslyngsoe
Copy link
Member

Hi @PerplexDaniel

Looking at your changes in src/Umbraco.Web/Editors/ContentController.cs, I start thinking that does not handle Segments since the code seems to only deal with Culture. Is that correctly understood? I'm trying to conclude whether we need to revisit this part later?

@PerplexDaniel
Copy link
Contributor Author

Yes, looking at that code we probably also need some segmentErrors to include those. Or perhaps just transform this cultureErrors into variantErrors and include errors on segmented variants. I only fixed a crash there and did not change any of the functionality.

@nielslyngsoe
Copy link
Member

Thanks for the clarification, I have noted that.

@nielslyngsoe nielslyngsoe merged commit 5a5291d into umbraco:v8/feature/AB4550-segments-ui-variant-picker Feb 18, 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 18, 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