Solves Issue #447 #802
Solves Issue #447 #802
Conversation
|
This solution for the collection tile is a problem :( |
|
WIP... Resolving conflicts |
|
YESSSS GREEN! Done! |
| self.config_fields = self.get_tile_configuration() | ||
| limit_conf = self.config_fields.get('count', None) | ||
|
|
||
| if limit_conf and 'size' in limit_conf.keys(): |
There was a problem hiding this comment.
limit_conf = self.config_fields.get('count', [])
if 'size' in limit_conf:
...
| form.omitted('count') | ||
| form.no_omit(IDefaultConfigureForm, 'count') | ||
| count = schema.Int( | ||
| count = schema.List( |
There was a problem hiding this comment.
Same comment as in collection https://github.com/collective/collective.cover/pull/802/files/39181f51604a56eae22ccf300c386012ad810081#r210260938
| number_to_show = schema.List( | ||
| form.omitted('count') | ||
| form.no_omit(IDefaultConfigureForm, 'count') | ||
| count = schema.List( |
There was a problem hiding this comment.
The schema says List, but you are working like if it was a dictionary.. could you please explain the idea behind this schema change? Why a simple Int don't resolve this problem?
There was a problem hiding this comment.
I only just kept the model already existing schema in the collection tile, but yes, a simple Int resolve this problem.
|
|
||
| if 'number_to_show' in tile_conf.keys(): | ||
| tile_conf['count'] = tile_conf['number_to_show'] | ||
| tile_conf.pop('number_to_show') |
There was a problem hiding this comment.
tile_conf['count'] = tile_conf.pop('number_to_show')
| tile = obj.get_tile(tile_id) | ||
| tile_conf = tile.get_tile_configuration() | ||
|
|
||
| if 'number_to_show' in tile_conf.keys(): |
There was a problem hiding this comment.
Please try to keep short indentation
if 'number_to_show' not in tile_conf:
continue
There was a problem hiding this comment.
@cleberjsantos I appreciate your interest on fixing this; I think we need to be be very careful and give a simpler and cleaner solution, even if it takes more time: having a schema field named count and making it inherit from List makes no sense to me neither.
also, we will need a test for the upgrade step.
fixes #802