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

Tags #4820

Merged
merged 41 commits into from Feb 20, 2020
Merged

Tags #4820

merged 41 commits into from Feb 20, 2020

Conversation

@deanmarcussen
Copy link
Member

deanmarcussen commented Nov 16, 2019

wip!

Fixes #2468

Currently just adds a custom tag editor for the taxonomy field.

TODO (and needs some design input)

  • Create tags on the fly (on save or during vue-multi-select?)
  • Limit Tag Taxonomy to single heirachy (needs TaxonomyPart to have settings, which means it needs to not be a code Part, but attachable, so you can access settings in the admin)
  • Create an initial SiteTags Taxonomy Content Item / w / Term Item. Probably via a feature
  • Helper to list tags by alias (i.e. SiteTags, or other created tag taxonomies)
  • More that I have forgotten

tags-initial

@agriffard

This comment has been minimized.

Copy link
Member

agriffard commented Nov 16, 2019

Do you also plan to add Tags/Categories on a BlogPost in the Blog recipe, by default, as I can see in the screenshot?

A link to the related taxonomy page could be useful next to the editor.

@deanmarcussen

This comment has been minimized.

Copy link
Member Author

deanmarcussen commented Nov 21, 2019

still wip (and blog theme may change with displayText additions still in progress), but screen shots anyway

added the tags to both the blog (homepage, and also the post itself. mostly to show the two different ways of rendering them, i.e. via the shape, or from the field data

blog (home page), render from field/ content item json
blog-tag

blog post, rendered from shape template
blog-post-tag

@agriffard

This comment has been minimized.

Copy link
Member

agriffard commented Nov 21, 2019

Good job @deanmarcussen .

Could we use two types of taxonomies in the sample: Category and Tags?
A Category would be a taxonomy with hierarchical fixed terms, selectable only once.
Tags would use the Tags editor and allow to add new terms.

Display them differently:
With a folder for the category.
With an icon https://fontawesome.com/icons/tag?style=solid for the tags.

The best use case will be to index those fields and be able to display them or search them by category or tag in a widget or a liquid page based on a query.

@deanmarcussen

This comment has been minimized.

Copy link
Member Author

deanmarcussen commented Nov 21, 2019

@agriffard yeah no worries. let me check what you mean though

What category would you name the blog post that is there?
and this icon for the category folder?
https://fontawesome.com/icons/folder?style=solid

Display the category in the same area as the tags are, but with the different icon to differentiate?

I was thinking to add a sql query example in already, so will do a couple, one for tags, and one for categories.

@sebastienros I am not totally convinced about adding DisplayText to the TaxonomyField data. It just feels like it's easy for the tag value to be changed, and the content not updated. And not that hard to call the terms liquid filter (to me)
but I fixed a bug there, when using the shape from liquid, so maybe that is why people think it is hard?
Or maybe they don't realise that the taxonomy content item will be cached, so think it is an expensive operation to call it?

}

// Resolve Model.Content.MyType-MyField-FieldName_Display__DisplayOption
return shape.Named(n);
}

This comment has been minimized.

Copy link
@deanmarcussen

deanmarcussen Nov 23, 2019

Author Member

@jtkech this is still a wip but if you have any time to review here it'd be appreciated.

I had to fix this to get Model.Content[BlogPost-Tags-TaxonomyField_Display__TagsDisplayText] to return the shape when I added a display mode for tags, as it creates quite a weird shape name when using display modes.

If you have any thoughts, I'm having trouble getting a template override working for it as well. The templates not crucial, but I think it's highlighting an issue with the display mode shape naming conventions.

TaxonomyField-TagsDisplayText.Display.liquid works, but would override for all of those shapes
BlogPost-TaxonomyField-TagsDisplayText.Display.liquid doesn't - I think that's probably the correct naming convention, but I'm not sure the shape table is handling the extra display mode options correctly (too many - getting swapped for __ or vice versa).

This comment has been minimized.

Copy link
@jtkech

jtkech Nov 24, 2019

Member

Yes the shape name may be set to a differentiator if not null or to the shapeType. The differentiatordepends on the type / part / field and most of the time it only contain a - separator. The shape type may start with a stereotype, may contain different things depending on the context e.g ContentPart_, here it uses the _ and __ separators, and it may contain a display mode, here the code.

    protected string GetDisplayShapeType(string shapeType, BuildFieldDisplayContext context)
    {
        var displayMode = context.PartFieldDefinition.DisplayMode();
        return !String.IsNullOrEmpty(displayMode)
            ? shapeType + "_Display__" + displayMode
            : shapeType;
    }

So here shapeType = TaxonomyField_Display__TagsDisplayText, and because it becomes different from the fieldType, in the ContentFieldDisplayDriver we do this

                result.Differentiator($"{partName}-{fieldName}-{shapeType}");

Which ends up with a shape Name = BlogPost-Tags-TaxonomyField_Display__TagsDisplayText.


Yes, shape.Named(n.Replace("__", "-")); has been added to allow the following in liquid, here tested with the standard display mode, otherwise as you it would not work.

Test1: {{ Model.Content.Article-Subtitle.Text }}
Test2: {{ Model.Content.Article__Subtitle.Text }}
Test3: {{ Model.Content["Article-Subtitle"].Text }}
Test4: {{ Model.Content["Article__Subtitle"].Text }}

Before we were able to do only this.

Test1: {{ Model.Content.Article-Subtitle.Text }}
Test3: {{ Model.Content["Article-Subtitle"].Text }}

Hmm, i think that would be okay, maybe i missed something.

But a binder has also been added at the end of Shape.cs, so that in razor you can use.

Test: @Model.Content.Article__Subtitle.Text // In place of using `Named()`

So, maybe this has been added mainly for razor where a property name can't contain a - character. And then the change in liquid allows to use the same field name with a double __.

But here it's not the same usage of the double __ than for a template name separator. Maybe we could use another character, but which one? I will try it tomorrow and think about it.


About alternates, just tried with an Articlewith a Subtitle in Header display mode that can be mapped to your case

displayType = "" i skip the one equal to "_Detail"
BlogPost = contentType = Article
BlogPost = partName = Article = contentType for a direct field
TaxonomyField = fieldType = TextField
Tags = fieldName = Subtitle
TaxonomyField_Display__TagsDisplayText = shapeType = TextField_Display__Header

In ContentFieldDisplayDriver in our case where shapeType != fieldType we add these alternates

{fieldType}{displayType}__{shapeType}
{partType}{displayType}__{fieldName}__{shapeType}
{contentType}{displayType}__{partName}__{fieldName}__{shapeType}
{contentType}{displayType}__{fieldType}__{shapeType}

which ends up with

TextField_Display__Header_Detail
TextField__TextField_Display__Header
Article__Subtitle__TextField_Display__Header
Article__Article__Subtitle__TextField_Display__Header
Article__TextField__TextField_Display__Header

So in your case

TaxonomyField_Display__TagsDisplayText_Detail
TaxonomyField__TaxonomyField_Display__TagsDisplayText
BlogPost__Tags__TaxonomyField_Display__TagsDisplayText
BlogPost__BlogPost__Tags__TaxonomyField_Display__TagsDisplayText
BlogPost__TaxonomyField__TaxonomyField_Display__TagsDisplayText

Which results in these files

TaxonomyField-TagsDisplayText.Display.liquid
TaxonomyField-TaxonomyField-TagsDisplayText.Display.liquid
BlogPost-Tags-TaxonomyField-TagsDisplayText.Display.liquid
BlogPost-BlogPost-Tags-TaxonomyField-TagsDisplayText.Display.liquid
BlogPost-TaxonomyField-TaxonomyField-TagsDisplayText.Display.liquid

So, BlogPost-TaxonomyField-TagsDisplayText.Display.liquid doesn't belong to the above list.

This is because of the duplicate segments, here partName = contentType for a direct field, and the shapeType starts with the fieldType. Maybe normal for the partName, at least we could check that the shapeType starts with the fieldType. Hmm, maybe we could just remove the duplicates or add more alternates without duplicates to not break existing templates.

So try one of the above alternates. About your change, right now i would suggest to do shape.Named(n) first. Maybe we would have also to change the binder at the end of the Shape.cs. I will think about it.

Didn't try any liquid alternate yet, maybe nothing works ;) i will try it tomorrow.

This comment has been minimized.

Copy link
@deanmarcussen

deanmarcussen Nov 24, 2019

Author Member

Thanks @jtkech I was looking for where those alternates where added and can see the issue clearly now.

About your change, right now i would suggest to do

I've done this change, but knowing you will think more about the binder.

So back onto the templates and alternates (noting that I refactored to Tags as the display mode, not TagsDisplayText - it made no sense)

I tried your suggestions for the alternates, but they don't work, because they don't match to a shape binding.
I've put a last commit in here, (a hack, and not intended to be the permanent fix, just to highlight what is going on)

Some definitions (to unconfuse myself)
displayType -> Summary or Detail
displayMode -> the special display mode that can be selected

So when using a display mode the shapeType becomes TaxonomyField_Display__Tags

But the binding for the template is always
blogpost_display__blogpost__tags__taxonomyfield__tags - I assume because the .Display is always placed at the second segment.

But because the shapeType actually contains the _Display it doesn't get morphed properly when adding the alternates.

So I did a quick hack to add some extra alternates when the shapeType contains _ and __

Maybe this is not such a bad hack, and just needs tweaking somewhat.
Or maybe another character other than _ or __ would make these display options less confusing, as you say? But what character indeed?

I think I've fixed this once now for display modes with placement. Maybe this is the last place that needs fixing, so not an issue to fix it similar to I have.

So thank you for all the great info, and I will let you think about it some more, about the best option.
If you want to push something to this pr, please feel free, or if you have better ideas, then on another pr, as you prefer.
This one is still not complete, and will need another demo at least :)

Also noting that the hack does nothing to remove what look to be useless alternates

@deanmarcussen

This comment has been minimized.

Copy link
Member Author

deanmarcussen commented Nov 24, 2019

@agriffard Have applied a taxonomy category to the recipe and templates.
For want of a better category, I chose Travel (and made a `Cooking category randomly, so there was more than one category by default). Option to ideas here ;) and on the placement of the tags and category.

I used an icon picker for the category term, so it's selectable. Mostly to demonstrate the usefulness of terms, that are also content items. But hard coded the icon for the tags, it seemed to make sense for the two different use cases.

blog-category

@jtkech

This comment has been minimized.

Copy link
Member

jtkech commented Nov 25, 2019

I pushed some testing code

So, _Display is not a display type but a kind of. For a given template file we can't have compound display type with more than one consecutive ., so the following works

BlogPost-Tags-TaxonomyField-Tags.Display.liquid

But this would not work

BlogPost-Tags-TaxonomyField-Tags.Display.Detail.liquid
BlogPost-Tags-TaxonomyField-Tags.Display.Summary.liquid

So my question is, when a field has a display mode, do we want to be able to display it in this mode and also at the same time in a given display type. E.g Tags and Detail or Tags and Summary and then having e.g at the end of a template file .Display.Detail.liquid or .Display.Summary.liquid.

That said the following works and still allows to make a differentiation.

BlogPost-Detail-Tags-TaxonomyField-Tags.Display.liquid <= i tried this one
BlogPost-Summary-Tags-TaxonomyField-Tags.Display.liquid <= not tried this one

Update
Or maybe loop over additional compound display types as DetailDisplay and SummaryDisplay.

@deanmarcussen

This comment has been minimized.

Copy link
Member Author

deanmarcussen commented Nov 25, 2019

Thanks @jtkech for tweaking that into something more suitable.

This working now is great!

BlogPost-Tags-TaxonomyField-Tags.Display.liquid

This not working is less great

BlogPost-Tags-TaxonomyField-Tags.Display.Detail.liquid
BlogPost-Tags-TaxonomyField-Tags.Display.Summary.liquid

but as you say you can get it working like

BlogPost-Detail-Tags-TaxonomyField-Tags.Display.liquid

My opnion on it, is that the template naming conventions are dificult enough to understand as it is, so it would be better if it was at least consistent.
But also it is a minor issue.

We only have one field with display options already (the TextField Header) and maybeif you were making a template override you might just override the TextField.Detail and use the standard display mode.

It would be confusing to say that only standard mode can have detail / summary overrides

Maybe the loop will work with it?

@deanmarcussen

This comment has been minimized.

Copy link
Member Author

deanmarcussen commented Nov 25, 2019

@agriffard pointed out that the blog recipe uses blog/post-1 as the default slug of the first blog.

Do we want to update it to use what would be the calculated value?
blog/man-must-explore-and-this-is-exploration-at-its-greatest

jtkech added 2 commits Nov 26, 2019
@jtkech

This comment has been minimized.

Copy link
Member

jtkech commented Nov 26, 2019

@deanmarcussen i didn't change the razor binder that allows to access an item as a property without using Named() this by using some double __ that will be replaced by dashes -. Anyway, when the item name already contains a double __, through razor we need to use Named().

Through liquid because we can use dashes for a property, and because of your change it works, so we can use e.g Model.Content.BlogPost-Tags-TaxonomyField_Display__TagsDisplayText, this in place of an array index Model.Content[BlogPost-Tags-TaxonomyField_Display__TagsDisplayText].

About field alternates with a display mode, all the following liquid files are now working.

// display mode and no display type
TaxonomyField-Tags.Display.liquid
TaxonomyField-TaxonomyField-Tags.Display.liquid
BlogPost-Tags-TaxonomyField-Tags.Display.liquid
BlogPost-BlogPost-Tags-TaxonomyField__Tags.Display.liquid
BlogPost-TaxonomyField-TaxonomyField-Tags.Display.liquid

// display mode and display type
TaxonomyField-Tags.Display.Detail.liquid
TaxonomyField-TaxonomyField-Tags.Display.Detail.liquid
BlogPost-Tags-TaxonomyField-Tags.Display.Detail.liquid
BlogPost-BlogPost-Tags-TaxonomyField__Tags.Display.Detail.liquid
BlogPost-TaxonomyField-TaxonomyField-Tags.Display.Detail.liquid
@deanmarcussen

This comment has been minimized.

Copy link
Member Author

deanmarcussen commented Nov 26, 2019

Thank you @jtkech your solution to the .Display .Detail issue is much more elegant than anything I had come up with. Much appreciated.

And yes, makes sense what you say about not changing the razor binder.

For me the change to the liquid accessor was required to access both through the array index method, i.e.

Model.Content.BlogPost-Tags-TaxonomyField_Display__TagsDisplayText and
Model.Content[BlogPost-Tags-TaxonomyField_Display__TagsDisplayText]

I think because the liquid accessor always used shape.Named() but first with the string.Replace, which changed out what the underlying shape was actually named, but only in the case of display modes.

@agriffard

This comment has been minimized.

Copy link
Member

agriffard commented Dec 15, 2019

What is the status on Tags?
May be you could transform your bulleted list in check boxes to show the remaining work.

@deanmarcussen

This comment has been minimized.

Copy link
Member Author

deanmarcussen commented Dec 15, 2019

Status on Tag is Ready to go, pending demo.

Haven't been able to make the meeting this past few weeks due to time constraints.
90% chance I'm going to make it this week, so will demo then.

Only outstanding point is about calling default editors, but that is resolved in this branch, and pending a further discussion about how to achieve that globally.

@deanmarcussen

This comment has been minimized.

Copy link
Member Author

deanmarcussen commented Dec 18, 2019

tested this recipe again, and it is working fine.

suspect a build problem in the meeting as I'd merged 4 branches into one to avoid having to rebuild for each demo.

Just tweaked the css a little bit now for better alignment.

Also responding to feedback. On the blog post page the tags are at the bottom, rather than in the header. Two reasons

  • They aren't very readable against the image backdrop that is in the header.
  • That was where medium places it's tag

Anyone else have time to check the recipe just to be sure?

…reating tags inline
# Conflicts:
#	src/OrchardCore/OrchardCore.DisplayManagement.Liquid/LiquidViewTemplate.cs
#	src/OrchardCore/OrchardCore.DisplayManagement/Shapes/Shape.cs
@agriffard

This comment has been minimized.

Copy link
Member

agriffard commented Jan 11, 2020

@arkadiuszwojcik

This comment has been minimized.

Copy link
Contributor

arkadiuszwojcik commented Jan 15, 2020

Any news on this?

@agriffard

This comment has been minimized.

Copy link
Member

agriffard commented Jan 15, 2020

@deanmarcussen How can we retrieve the content items associated to a taxonomy term?
It seems that there is nothing that exists to list the blog posts tagged with a specific term.

What kind of helper would allow you to do this?

  • A YesSql query on the TaxonomyIndex.
  • Index the field, search the terms in Lucene and retrieve them in Liquid.

Then, how do we make them available:

  • Liquid filter on a string.
  • Method in @OrchardCore razor helper.

Examples of expected scenarios:

  • SQL query that retrieves all distinct terms of a taxonomy (and the number of times they are used)
  • Find a way to get a term from an url (Ex: /tags/myTerm).
  • For each term of a taxonomy, display the associated content items.
@deanmarcussen

This comment has been minimized.

Copy link
Member Author

deanmarcussen commented Jan 15, 2020

@arkadiuszwojcik we're sorting out the driver registration first.

@agriffard do you mean this helper QueryCategorizedContentItemsAsync which is on the razor helper?

It's the one I use when retrieving content items based on a term.

But it is not available as a liquid filter, because it takes a predicate, which is difficult to transpose in liquid.

I think you're correct for this PR I should also include a SQL query in the recipe showing how to retrieve from the taxonomy index.

Which could then be used from liquid.

The other issues you listed below are separate issues that should be addressed on a different pr.

  • Find a way to get a term from an url (Ex: /tags/myTerm).
  • For each term of a taxonomy, display the associated content items.

My personal preference for these is different to Seb's suggestion of using the TermContentId in the url and having a separate controller.

I prefer to create a totally different content item, with the term route that I want, and rendering the taxonomy field in such a way that it queries for the associated terms, as I demonstrated late last year. For that I did have a custom display option for the taxonomy field which uses the QueryCategorizedContentItemsAsync helper

@agriffard

This comment has been minimized.

Copy link
Member

agriffard commented Jan 15, 2020

Would you be able to add an example to show how you use QueryCategorizedContentItemsAsync?
May be in the documentation: http://docs.orchardcore.net/en/dev/docs/reference/modules/Taxonomies/#querycategorizedcontentitemsasync

# Conflicts:
#	src/OrchardCore.Modules/OrchardCore.Taxonomies/Drivers/TaxonomyFieldDriver.cs
#	src/OrchardCore.Themes/TheBlogTheme/Recipes/blog.recipe.json
# Conflicts:
#	src/OrchardCore/OrchardCore.DisplayManagement/Shapes/Shape.cs
@deanmarcussen

This comment has been minimized.

Copy link
Member Author

deanmarcussen commented Feb 9, 2020

@agriffard if you have any time this recipe could do with a test run (lot of merge conflicts to resolve in it!)

I have not done a sql query at this stage. Another pr, after we look more at how the routing can work etc

But I think this is ready for review @sebastienros as the drivers are now registered via options (and it works well!) but a second look at how I have done the TagElement would be good

@agriffard

This comment has been minimized.

Copy link
Member

agriffard commented Feb 9, 2020

OK, I tested the Blog recipe.

A few remarks:

  • Even if the category is Unique, may be we can keep the BlogPost-Category.liquid iterating on all the terms of the Category, in case people want to change it to have multiple categories.
  • The TaxonomyField with the Tags editor and the Required setting doesn't seem to display an error message on saving the content item when it is empty.
  • I tested to a a new 'Tag' tag on a blog post and then to delete this added term in the Tags taxonomy. No errors but the blog post is still displaying the term, but I guess it is complicated to remove the associated terms when the term is removed.
  • I tried to add terms in different orders in a blog post but when they are rendered or re-edited, the taxonomy order is used. I guess it is on purpose.
@agriffard agriffard self-requested a review Feb 9, 2020
@deanmarcussen

This comment has been minimized.

Copy link
Member Author

deanmarcussen commented Feb 9, 2020

Thanks Antoine

  • Even if the category is Unique, may be we can keep the BlogPost-Category.liquid iterating on all the terms of the Category, in case people want to change it to have multiple categories.

Done

  • The TaxonomyField with the Tags editor and the Required setting doesn't seem to display an error message on saving the content item when it is empty.

Done

  • I tested to a a new 'Tag' tag on a blog post and then to delete this added term in the Tags taxonomy. No errors but the blog post is still displaying the term, but I guess it is complicated to remove the associated terms when the term is removed.

Yes, this is by design, and was one of the compromises of using taxonomies with tags in this way.
We could have an update items iterator, but that was outside the design scope.

  • I tried to add terms in different orders in a blog post but when they are rendered or re-edited, the taxonomy order is used. I guess it is on purpose.

Yes, this is on purpose. Control is mostly held with the taxonomy itself

Copy link
Member

agriffard left a comment

Blog recipe tested.

Content definitions correctly set.
Taxonomy Field Settings are working for all editors.
Admin page allows to edit the taxonomies and the terms.
Summary and Details views display the Category and the Tags as expected.

public static void SetTagNames(this TaxonomyField taxonomyField, string[] tagNames)
{
taxonomyField.Content["TagNames"] = JArray.FromObject(tagNames);
}

This comment has been minimized.

Copy link
@deanmarcussen

deanmarcussen Feb 16, 2020

Author Member

@sebastienros implementing the TagNames ContentElement as an IEnumerable does not work because the elements dictionary expects JObject and won't take a JArray

eg

elementData = JObject.FromObject(element, ContentBuilderSettings.IgnoreDefaultValuesSerializer);

So implemented instead directly against the Content property

Which makes the accessor in liquid Model.ContentItem....TaxonomyFieldName.TagNames

@deanmarcussen deanmarcussen mentioned this pull request Feb 18, 2020
@sebastienros sebastienros merged commit d599a9a into dev Feb 20, 2020
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
license/cla All CLA requirements met.
Details
@sebastienros sebastienros deleted the deanmarcussen/tags branch Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.