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

Autoroute container routing #5665

Merged
merged 22 commits into from
Apr 11, 2020
Merged

Autoroute container routing #5665

merged 22 commits into from
Apr 11, 2020

Conversation

deanmarcussen
Copy link
Member

Fixes #2688

Replaces #5455

Merged all required settings into Autoroute.

# Conflicts:
#	src/OrchardCore.Modules/OrchardCore.Flows/Drivers/BagPartDisplay.cs
@deanmarcussen deanmarcussen marked this pull request as ready for review March 7, 2020 16:07
@deanmarcussen
Copy link
Member Author

@agriffard if you had any time to try this, it'd be appreciated.

Any better hint texts would also be helpful ;)

I haven't as yet included autoroute on the taxonomy content type by default (probably it should be not sure?) so you would have to add it.

I haven't updated the blog recipe or templates either, there are enough changes on this pr, I would do that on another pr

@agriffard
Copy link
Member

Suggestion:
In order to clearly distinguish them from other settings, wrap the settings that are related to 'Contained items' in a fieldset with a legend like 'Container':

image

Copy link
Member

@agriffard agriffard left a comment

Choose a reason for hiding this comment

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

I love it.
If you find that my remark about a Container fieldset can add some value, please add it.
It is just a bit difficult to understand the impact of the different settings just thanks to the hints.

Can't wait to see some documentation (guide or in taxonomies module) and more integration in the BlogTheme, event it is in other PRs.

@agriffard
Copy link
Member

The biggest challenge is to make it understandable for everyone.
Ex: I am sure I will be struggling to find the good translations in French.

As I see your discussion about Content tree, it makes me think that we should may be not present the feature as a Container/Contained items but as a Tree/Nodes/Leaves or as a Parent/Children relationship.

Ex: A route that 'Can have children' is easier to understand than 'Can have contained items'.

@deanmarcussen
Copy link
Member Author

Good timing @agriffard I was just putting the field set in.

You are right. The hardest part of this pr is make the ui...

So I used Contained / Container because it is common Orchard terminology from O1 and also OC, but no one ever understands it (at first).

And as the words are so similar, it is hard to differentiate.

I am lazy so I would like to keep the code referring to Contained / Container. It makes more sense there I think, than Parent / Child (because sometimes a child can also be a parent when things are nested...)

Tree/Node/Leaves works well for taxonomies (but is also quite Orchard specific), but doesn't make so much sense for BagParts.

So to keep the UI a little bit similar to the code, how about I use Contained / Container for the Properties, but make the hints refer to parent / child.

Example for settings:
settings

And on the content item itself:

parent

Open to any more suggestions to make this more easily understandable :)

And is it translatable?

@agriffard
Copy link
Member

Exactly what I was expecting.

if (_autorouteEntries.TryGetEntryByContentItemId(context.ContentItem.ContentItemId, out var entry) &&
!string.IsNullOrEmpty(entry.ContainedContentItemId))
{
metadata.DisplayRouteValues = new RouteValueDictionary {
Copy link
Member Author

Choose a reason for hiding this comment

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

one side effect is that if contained item routing is not enabled, generating a route for it with a url helper will generate a standard route (i.e. /Contents/Item/id) which will 404

Copy link
Member Author

@deanmarcussen deanmarcussen left a comment

Choose a reason for hiding this comment

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

This is ready for a review of design choices @sebastienros

I have also deleted a number of files from taxonomies that probably came across from menus and are unused.

Made a Term and TermItem shape similar to menu shapes, which are populated by drivers, and the TermPart. This should enable most common uses to be overridden in liquid without having to make queries, as they are populated by drivers / other shapes.

I have added the Autoroute part as part of migrations to taxonomies, and check that it is non breaking. Will require a republish and applying settings to work.

There would be a few follow up pr's, related to the blog theme templates, and probably a similar concept for the AliasPart, and docs etc, once design choices are ratified

@agriffard agriffard requested a review from sebastienros March 16, 2020 10:42
@deanmarcussen
Copy link
Member Author

Little bit more progress here. @agriffard the blog theme is updated now :)

It could do with another test drive if you have time.

Here's some screen shots

categories
travel
tags

All the badges now have links to their category or tag

There is now a Term shape, similar to the Menu shape that can be called from either liquid or razor to render a taxonomy by Alias or TaxonomyContentItemId, and if supplied a TermContentItemId which will reduce the list of Terms to the hierarchy beginning at the level of the TermContentItemId

Model.Metadata.Alternates.Clear();
Model.Metadata.Type = "TermContentItem";

tag.InnerHtml.AppendHtml(await DisplayAsync(Model));
Copy link
Member

Choose a reason for hiding this comment

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

Not obvious that using InnerHtml is necessary. Looks like it could just work with some direct calls to @await displayAsync. Unless you found a reason due to ordering that was not possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

No reason. I just hadn't done it that way. Have removed the tagbuilder and done directly with @await DisplayAsync()


The display for the `Taxonomy` is then rendered by the `TaxonomyPart` shape.

This uses the `TermShape` to display a heirachy of the `Terms`
Copy link
Member

Choose a reason for hiding this comment

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

heirachy => hierarchy


The `TermPart` is rendered when a `Term` is displayed with the `Container routing` feature of the `AutoroutePart`.

It renders a list of all content items that have been categorized by the `TaxonomyField` as part of that `Term` hierachy.
Copy link
Member

Choose a reason for hiding this comment

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

hierachy => hierachy


### Term Shape

The `TermShape` is used by the `TaxonomyPart` display to render the list of term hierachies for the `Taxonomy`.
Copy link
Member

Choose a reason for hiding this comment

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

hierachies => hierarchies


The `TermShape` is used by the `TaxonomyPart` display to render the list of term hierachies for the `Taxonomy`.

It is a reusable shape which may also be called from a content item, similar to that of a `MenuShape`, to render either the entire `Taxonomy` and term hierachy, or a part of the `Term` hierachy.
Copy link
Member

Choose a reason for hiding this comment

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

hierachy => hierarchy twice

<shape type="Term" alias="Categories" />
```

You can also specify a `TermContentItemId` to render a part of the term hierachy.
Copy link
Member

Choose a reason for hiding this comment

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

hierachy => hierarchy

@agriffard agriffard merged commit 95b3301 into dev Apr 11, 2020
@delete-merged-branch delete-merged-branch bot deleted the deanmarcussen/autoroute-container branch April 11, 2020 16:32
scleaver pushed a commit to kast-cloud/OrchardCore that referenced this pull request Apr 15, 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.

Find a way to display contained content items
4 participants