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

Issue U4-6585 - add textbox for additional grid classes to row config cells #678

Closed
wants to merge 2,995 commits into
base: dev-v7
from

Conversation

Projects
None yet
@marcemarc
Contributor

marcemarc commented May 4, 2015

Not sure if this is complicating row.config too much, but it has made it easier for us to implement a responsive design using the grid, by configuring the mobile / large screen responsive grid classes in the row.config, rather than in code. Also is 'GridClasses' even the correct name etc..

@tonypye

This comment has been minimized.

tonypye commented Aug 2, 2016

Mark, this is an awesome change that I've just implemented in a client project that has completely solved my dilemma; how to not rely on the content editor to pick the right class from a bit long list that covers lots of different row and/or cell config scenarios. I've also expanded on this to have an additional text field below the 'row' name for css classes that can be added to the row by default, giving real flexibility.
I can't believe this hasn't been included in the core yet...and something so simple to alter.

@marcemarc

This comment has been minimized.

Contributor

marcemarc commented Sep 25, 2016

@tonypye glad it was helpful, there is an issue on the issue tracker for it, http://issues.umbraco.org/issue/U4-6585 if you would like to add your vote! I think it needs to get 20+ votes for the @umbraco/umbraco-core team to investigate, but you only see it's a problem when you implement a large site with the grid...

@Shazwazza

This comment has been minimized.

Member

Shazwazza commented Oct 17, 2016

@marcemarc I haven't tested this out yet but I have a concern so please let me know if this is the case. From the description of this change on the tracker it suggests that these cell class values are stored via pre-values for the grid?

If that is the case, there will be more work required to do this correctly. We had this problem with the initial grid implementation in the past where grid pre-value values ended up being stored with the actual property value for grids, which simply cannot occur. The reason is because pre-values are a single source value that cannot be duplicated between property values, so that if you make a change to pre-values, it is reflected for all property values without having to go re-save every document that contains the grid.

There's already code in the core that performs the merging of pre-values for grid property values, so if this is in fact storing these classes in pre-values, you'll need to follow this same pattern for merging when the property values are resolved. You also need to follow the other pattern in the core that filters out these pre-values from being persisted with the actual property value.

@marcemarc

This comment has been minimized.

Contributor

marcemarc commented Oct 17, 2016

@Shazwazza yes, but the idea is right I think? - essentially the point at which you configure a row will be 3 - 3 - 3 - 3 columns is also the point at which you want to configure, how those cells will be rendered on smaller screens, rather than expecting editors to add css classes or having to update your grid rendering file to add them programatically.

So in the pull request these additional responsive classes for each cell to implement your responsive design are saved on the currentCell, in the same way that the 'number' for the cell column is saved.

But I can see your point that currently if at some later date, someone comes into a site that uses the grid with a row that has been used on a hundred pages, and decides this 'product row' now needs to be 4 - 4 - 4, perhaps deleting a 4th cell from the row then you won't see this change on the front end of your site until you open each of those hundred pages and save and publish. In fact people could lose data by removing a cell. (should there be a warning if you delete a cell from a row config ?)

and this additional css class configuration follows this same pattern, because it is added at the cell configuration stage, it is part of the definition of that cell and it becomes a problem at a later date when you come to implement a 'new design' on an existing site and you want to keep the same content but change the layout of the grid columns and perhaps set different default responsive css classes for this new design ?

I hadn't really considered the migration of existing grid content into a new grid layout on a large site, and can see how people might expect to be able to change the config and have the site immediately updated as per traditional page templating - and I can see how storing the row config column sizes alongside the content would be a problem with this kind of change, let alone the additional css classes I have proposed.

So I'm guessing your concern is your looking for a way to take out the grid column number from the stored content, not add more stuff to it !!!

(but it's great if you don't have to change your design at a later date!)

nul800sebastiaan and others added some commits Feb 23, 2018

Added checks in place to stop users that has no access to sensitive d…
…ata to export member data, removed some unused usings
Merge pull request #2471 from alanmac/U4-11007
change newsletter subscribe to default for installation process - GDPR Compliance
U4-10964 Adding a new property to a MemberType if you are not in Sens…
…itive Data group throws a 404 when saving
Some more code cleanup and making sure we pass in the current user so…
… that we can't avoid the sensitivedata check easily
Merge pull request #2469 from umbraco/temp-U4-10900
Export member data functionality
Merge remote-tracking branch 'origin/dev-v7' into dev-v7.8
# Conflicts:
#	src/Umbraco.Web/Mvc/RenderRouteHandler.cs
Merge pull request #2445 from bjarnef/dev-v7-U4-10966
U4-10966 -  Single/Multiple Media Picker is messed up when dragging images
fix xml in spanish lang file
(cherry picked from commit 2281cd8)
Merge remote-tracking branch 'origin/dev-v7' into dev-v7.8
# Conflicts:
#	src/SolutionInfo.cs
#	src/Umbraco.Core/Configuration/UmbracoVersion.cs
#	src/Umbraco.Web.UI.Client/src/views/propertyeditors/mediapicker/mediapicker.html
Merge remote-tracking branch 'origin/dev-v7.8' into dev-v7.9
# Conflicts:
#	src/SolutionInfo.cs
#	src/Umbraco.Core/Configuration/UmbracoVersion.cs
U4-10688 Obsolete property editors showing
Please be aware that if you do have obsolete property editors in use and set the setting "showDeprecatedPropertyEditors" to false you'll get an exception.

Shazwazza and others added some commits Apr 3, 2018

Fixes all references to assetsService.load* that do not pass in a req…
…uired scope object and changes assets.service to always use the $rootScope of an explicit scope is not passed
Merge branch 'dev-v7-U4-6585' of https://github.com/marcemarc/Umbraco…
…-CMS into dev-v7-U4-6585

# Conflicts:
#	src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/dialogs/rowconfig.html
merge in grid cell css classes set in configuration, so that if they …
…change in the configuration of the grid cell, they overwrite any values persisting in content that were published before the change
Resolve conflicts in grid renderer files from previous PR (renamed pr…
…operty to gridCellCssClasses to be more descriptive)
merge in grid cell css classes set in configuration, so that if they …
…change in the configuration of the grid cell, they overwrite any values persisting in content that were published before the change
Merge branch 'dev-v7-U4-6585' of https://github.com/marcemarc/Umbraco…
…-CMS into dev-v7-U4-6585

# Conflicts:
#	src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/dialogs/rowconfig.html
@marcemarc

This comment has been minimized.

Contributor

marcemarc commented Apr 4, 2018

Thanks @Shazwazza

I had no idea it was doing 'THAT' with the grid Json in the GridValueConverter, so now your original comment makes sense to me, I think.

So I've updated the PR

but I think I've somehow mucked up the rebase of this, so if it's easier created a new PR here: #2559

So hopefully that's more along the lines of what you were thinking for making this work, bits I'm not too sure about:

  1. is it ok to pull the prevalues like this in the gridvalueconverter file
  2. Have used SelectToken to lookup the config in the for the particular row, again not too sure if this is the best way.

regards

marc

@nul800sebastiaan

This comment has been minimized.

Member

nul800sebastiaan commented Jun 19, 2018

Closing this in favor of the newer version #2559

@marcemarc

This comment has been minimized.

Contributor

marcemarc commented Jun 19, 2018

@nul800sebastiaan but now I don't have the 'oldest' open PR :-P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment