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

Upgrade step for simplified data layout (simplify_layout) corrupts covers in existing setups #530

Closed
fredvd opened this issue Jun 18, 2015 · 9 comments
Labels
Milestone

Comments

@fredvd
Copy link
Member

fredvd commented Jun 18, 2015

commit f740406 changes the structure of existing covers/layouts where all the column sizes seem to be reset to 1 column. Will investigate later today, have a meeting first.

Before:
before

After:
after_gif

@hvelarde
Copy link
Member

@rodfersou can you please take a look?

@hvelarde hvelarde added the bug label Jun 18, 2015
@rodfersou
Copy link
Member

@fredvd could you share this cover layout with us to investigate what happen?

@fredvd
Copy link
Member Author

fredvd commented Jun 18, 2015

Here's a dump of obj.cover_layout before and after the upgrade. (http://www.vmm.be front page)

Before update:

'[{"type": "row", "children": [{"data": {"layout-type": "column", "column-size": 12}, "type": "group", "children": [{"tile-type": "collective.cover.carousel", "type": "tile", "id": "ad5449954e5c49ba9af021e7b20a4614"}], "roles": ["Manager"]}]}, {"type": "row", "children": [{"data": {"layout-type": "column", "column-size": 6}, "type": "group", "children": [{"tile-type": "vmm.tiles.lookingfor", "type": "tile", "id": "d10e3a2de5ee42bcb361f37c48ca4e8b"}], "roles": ["Manager"]}, {"data": {"layout-type": "column", "column-size": 6}, "type": "group", "children": [{"tile-type": "vmm.tiles.lookingfor", "type": "tile", "id": "c0d64b508b024643aa5fd1e5c1ad7559"}], "roles": ["Manager"]}]}, {"type": "row", "children": [{"data": {"layout-type": "column", "column-size": 6}, "type": "group", "children": [{"tile-type": "collective.cover.richtext", "type": "tile", "id": "b7a0d4dbde2f4ca69eedf4496eebd3bb"}, {"tile-type": "collective.cover.list", "type": "tile", "id": "b49b9014a40541e286dabaafc8630a6a"}, {"tile-type": "collective.cover.list", "type": "tile", "id": "c96f7d594c80416f93cc0cd790f08964"}], "roles": ["Manager"]}, {"data": {"layout-type": "column", "column-size": 6}, "type": "group", "children": [{"tile-type": "collective.cover.richtext", "type": "tile", "id": "ce24af8797af437bb581da13e4d48708"}], "roles": ["Manager"]}]}]'

2015-06-18 19:39:44 INFO collective.cover "/plone/welkom" was updated

After update:

u'[{"type": "row", "children": [{"type": "group", "children": [{"tile-type": "collective.cover.carousel", "type": "tile", "id": "ad5449954e5c49ba9af021e7b20a4614"}], "roles": ["Manager"], "column-size": 12}]}, {"type": "row", "children": [{"type": "group", "children": [{"tile-type": "vmm.tiles.lookingfor", "type": "tile", "id": "d10e3a2de5ee42bcb361f37c48ca4e8b"}], "roles": ["Manager"], "column-size": 6}, {"type": "group", "children": [{"tile-type": "vmm.tiles.lookingfor", "type": "tile", "id": "c0d64b508b024643aa5fd1e5c1ad7559"}], "roles": ["Manager"], "column-size": 6}]}, {"type": "row", "children": [{"type": "group", "children": [{"tile-type": "collective.cover.richtext", "type": "tile", "id": "b7a0d4dbde2f4ca69eedf4496eebd3bb"}, {"tile-type": "collective.cover.list", "type": "tile", "id": "b49b9014a40541e286dabaafc8630a6a"}, {"tile-type": "collective.cover.list", "type": "tile", "id": "c96f7d594c80416f93cc0cd790f08964"}], "roles": ["Manager"], "column-size": 6}, {"type": "group", "children": [{"tile-type": "collective.cover.richtext", "type": "tile", "id": "ce24af8797af437bb581da13e4d48708"}], "roles": ["Manager"], "column-size": 6}]}]'

@rodfersou
Copy link
Member

As you can see in this image, it just change the way column-size are applied:
diff
That's exactally what should happen in this upgrade step, so I don't know what is the problem.

@rodfersou
Copy link
Member

The layout-type property is not necessary, so it was removed.

@rodfersou
Copy link
Member

The code of c.cover should understand the new structure

@hvelarde
Copy link
Member

@fredvd could you confirm this?

@fredvd
Copy link
Member Author

fredvd commented Jul 9, 2015

The problem was that our custom Grid system wasn't using the internally changed column-size yet so all columns were 'reset' to size 1. I already figured out if all else was ok our only other customisation was the gid system (heck we added customisable grid to cover :-P ). and found @djowett's updated to the developer docs.

I've added a warning to the developer docs that 1.0a10 > needs an update to custom grid systems and i've put it in the changes so other people can see this. @rodfersou even if it's internal data changes, please mention backward incompatible changes in the main CHANGES log with a warning, it's the only way contributors to c.cover can figure out incompatible changes without hunting through all the changes on github themselves. IGridSystem is an 'official' extension for integrators to configure cover afaics.

fredvd added a commit that referenced this issue Jul 9, 2015
that existing custom grid systems have to be upgraded after release 1.0a11 because of internal data structure changes, otherwise your cover columns will be seem to reset to  width "1". (closes `#530`_).
@fredvd
Copy link
Member Author

fredvd commented Jul 9, 2015

Closes by fixes to a custom GridSystem as per the updates found in b63ff67

@fredvd fredvd closed this as completed Jul 9, 2015
@hvelarde hvelarde added this to the 1.0a12 milestone Oct 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants