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

Normalize content type names for 2.0 #3624

Closed
alexander-schranz opened this issue Nov 15, 2017 · 14 comments
Labels
Milestone

Comments

@alexander-schranz
Copy link
Member

@alexander-schranz alexander-schranz commented Nov 15, 2017

Q A
Bug? no
New Feature? no
Sulu Version for 2.0

Actual Behavior

We have some multi selection content types like snippet, form_select, media_selection, category_list. Also the internal_links content type should be renamed to something which match more its case page_selection or content_selection.

Expected Behavior

This content types should be normalized to e.g.: <type>_selection or <type>_list

Here a list of all content types and which should be maybe renamed

Current Name Expected Name Implemented
text_line [x]
text_area [x]
text_editor [x]
resource_locator [x]
block [x]
contact contact_account_selection (#4566) [ ] (ui missing)
contact_selection [ ]
account_selection [ ]
media_selection [x]
category_list category_selection (#4335) [x]
snippet snippet_selection (#4565) [x]
smart_content [x]
teaser_selection (#4528) [x]
internal_links page_selection (#4500) [x]
single_internal_link single_page_selection (#4500) [x]
phone [x]
password [x]
url [x]
email [x]
date [x]
time [x]
color [x]
checkbox toggler? (#3992) [x]
multiple_select select (#4260) [x]
single_select single_select [x]
tag_list tag_selection (#4334) [x]
location (#4743) [x]
route resource_locator (#4594) [x]
article_selection sulu/SuluArticleBundle#377 [x]
page_tree_route [ ]
form_select form_selection [ ]

Other components which exists currently as component but not as content type are:

Name Implemented
single_contact_selection [x] (#4162)
single_media_upload [ ] (content type not needed I think)
single_account_selection [x] (#4489)

Also we should use the ParametersBag of symfony in the content type manager as he will output a Did you mean ... error message when someone did make a mistake when typing a content type.

Possible Solutions

Single:

  • A single_page (single_<type_singular>)
  • B page (<type_singular>) (problem with media, news when using <type> as single)
  • C single_page_selection (single_<type_singular_selection>)

Multiple:

  • 1 page_selection (<type_singular>_selection)
  • 2 page_list (<type_singular>_list)
  • 3 multiple_page (multiple_<type_singular>)
  • 4 pages (<type_plural>) (problem with media, news when using <type> (B) as single)
  • 5 pages_selection (<type_plural>_selection)
  • 6 pages_list (<type_plural>_list)
@alexander-schranz alexander-schranz added this to the Release 2.0 milestone Nov 15, 2017
@alexander-schranz

This comment has been minimized.

Copy link
Member Author

@alexander-schranz alexander-schranz commented Nov 15, 2017

When thinking about it <type>_list makes for me more sense as <type>_selection that it tells me that the response of that content type is a list of items and not a single item.

/cc @danrot @chirimoya @sulu/core-team

@alexander-schranz alexander-schranz changed the title Normalize content types for 2.0 Normalize content type names for 2.0 Nov 15, 2017
@wachterjohannes

This comment has been minimized.

Copy link
Member

@wachterjohannes wachterjohannes commented Nov 20, 2017

@alexander-schranz a selection says that it can be selected. a list in my opinion is more about show me the first 100 (as an example) and a pagination.

But the original indentation of this issue is really needed and should also be done for teaser and data-provider

@danrot

This comment has been minimized.

Copy link
Member

@danrot danrot commented Nov 20, 2017

First of all, a huge +1 on the ParameterBag. Would really help.

The thing with both suggestions is that it is still not clear how to name the single variants... So when internal_links become page_selection, what will single_internal_link be? Maybe remove the suffix completely, and only have page and pages? But that might also be confusing 😕 What about single_page and page?

@trickreich

This comment has been minimized.

Copy link
Contributor

@trickreich trickreich commented Nov 20, 2017

In my opinion there is also another possibility:
Don't make a difference between one or more, this should be done via parameter of the content type itself: max and min for example

@alexander-schranz

This comment has been minimized.

Copy link
Member Author

@alexander-schranz alexander-schranz commented Nov 20, 2017

@danrot completely removing the suffix for single and multi selection will not working or be confusing for things like news, media, ... which are plural words.

Another solution would be always prefixing it like it is done for multiple_select and single_select.

  • multiple_page
  • multiple_media
  • multiple_category
  • ...
  • single_page
  • single_media
  • ...

But feels not very comfortable for me.

@trickreich I agree that we need something like that for the multiple content types but a single selection content type should be optimized in the frontend for that use case and has another UI/UX as a multi selection content type.

@danrot

This comment has been minimized.

Copy link
Member

@danrot danrot commented Nov 20, 2017

@trickreich The other thing is that just changing a parameter should not break too much, which would be the case with this (when just removing the single parameter it would expect a single value instead of an array in the database).

@alexander-schranz I think I would go with a default then instead. It feels like there'll be too much clutter otherwise. But it is also not very optimal, any other suggestions?

@wachterjohannes

This comment has been minimized.

Copy link
Member

@wachterjohannes wachterjohannes commented Nov 20, 2017

what about media for the single and media_selection for the list? furthermore, we should try to keep the database value the same for single and multiple selections (use array in the database) and normalize that in the content-type. that would avoid crashing the request so often when changing the content-type

@alexander-schranz

This comment has been minimized.

Copy link
Member Author

@alexander-schranz alexander-schranz commented Nov 20, 2017

@wachterjohannes I would not use always array. As in a contact edit I only have 1 media as avatar and sending then an array of medias to the contact api seems to be not correct for me. That the template crashes at the read method of a content type is another issue and could be avoid be checking for is_string and is_array in the different content types.

I added the different solutions to the issue description feel free to extend the list.

@wachterjohannes

This comment has been minimized.

Copy link
Member

@wachterjohannes wachterjohannes commented Nov 20, 2017

@alexander-schranz that could be normalized in the content-type (only the value in the database should be the same type)

@danrot

This comment has been minimized.

Copy link
Member

@danrot danrot commented Nov 23, 2017

I wouldn't say this is necessary, since this goes further than just the value in the database. If we change the content type from a single to a multiple one twig templates are gonna also crash, so that's only delayed.

@wachterjohannes

This comment has been minimized.

Copy link
Member

@wachterjohannes wachterjohannes commented Nov 24, 2017

@danrot but that can be changed by the developer on the development machine without touching the data on production.

@danrot

This comment has been minimized.

Copy link
Member

@danrot danrot commented Nov 27, 2017

Of course all of this can be fixed by the developer, but IMO that's bad DX.

@alexander-schranz

This comment has been minimized.

Copy link
Member Author

@alexander-schranz alexander-schranz commented May 23, 2018

As discussed today with @danrot @chirimoya we postfix multi selections with: _selection e.g.: page_selection, contact_selection, ... and we prefix the single selections with single_ e.g.: single_page_selection

The selects will change to:
multiple_select -> select
single_select -> single_select

The table in the issue description was updated with the new preferred values.

@danrot

This comment has been minimized.

Copy link
Member

@danrot danrot commented Nov 7, 2019

All the content types have been renamed 🎉

@danrot danrot closed this Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.