Introduce layers in designer #6229
Conversation
| @@ -164,13 +164,31 @@ | |||
| </div> | |||
| {% endif %} | |||
| </div> | |||
| <div class="designer-tools-row js-hide-on-flip"> | |||
| <div class="tool"> | |||
| <span class="f-self-stretch">Layer visibility</span> | |||
There was a problem hiding this comment.
pretty much all the strings are missing i18n atm
tomako
force-pushed
the
introduce_layers_in_designer
branch
from
6e298b8
to
be66513
Compare
| def _remove_invisible_items(self, tpl_data): | ||
| pos_to_remove = [i for i, item in enumerate(tpl_data.items) if not tpl_data.layers[item['layer']]] | ||
| for pos in reversed(pos_to_remove): | ||
| tpl_data.items.pop(pos) |
There was a problem hiding this comment.
I think this is dangerous: AFAICT items is passed through directly from the dict that's on the model, so the only reason why this does not affect the stored data is that there's no mutation detection on mutable sqlalchemy data.
I think this is a safer alternative (untested):
tpl_data = tpl_data._replace(items=[item for item in tpl_data.items if tpl_data.layers[item['layer']]])There was a problem hiding this comment.
Oops, indeed. However _replace creates a new instance and self.tpl_data will still refer to the old one. (side note: passing instance attributes to _draw_badge method seems unnecessary)
Actually it would be more reasonable to remove all invisible items only once right at the beginning. However that would break our intention to manipulate item's visibility later per registrations.
So eventually I wouldn't do any destructive modification only skipping invisible items when placing them on the canvas.
| <div class="designer-tools-row second-row disappear"> | ||
| <div class="element-tools hidden"> | ||
| <div class="tool"> | ||
| <span class="selection-text f-self-stretch"></span> | ||
| <button class="js-remove-element i-button icon-remove icon-only text-color subtle" | ||
| title="{% trans %}Remove element{% endtrans %}"></button> | ||
| </div> | ||
| <div class="tool"> | ||
| <select id='item-layer-selector' class="js-layer-tool" data-attr="layer"> | ||
| <option value="1">{{ gettext('Layer {0}').format(1) }}</option> |
There was a problem hiding this comment.
{% trans %}Layer 1{% endtrans %}
No need to try to use the same gettext pattern as in the JS since JS and python/jinja are separate i18n dicts anyway so they have to be translated twice in any case
There was a problem hiding this comment.
Ah, I see. But I tried hard :)
There was a problem hiding this comment.
FWIW a cleaner way to do this (if it wasn't for the separate dicts) would be this: {% trans n=1 %}Layer {{ n }}{% endtrans %}
(generally using gettext in templates directly is somewhat uncommon)
There was a problem hiding this comment.
Ok, thanks. My motivation was indeed to use the same form as in js.
tomako
force-pushed
the
introduce_layers_in_designer
branch
from
be66513
to
f40bab2
Compare
|
Just to understand, does this layers functionality do anything by itself in Indico for someone who's not using your UN plugin? Alternatively I'd be OK with making it opt-in e.g. via a variable coming from a template hook so by default this would not show up and everything would go in the same layer/group. PS: You need to rebase and fix some conflicts, since we merged another PR related to badges. |
tomako
force-pushed
the
introduce_layers_in_designer
branch
from
f40bab2
to
1357abc
Compare
|
I very agree in that, it would be a bit weird :) Let me clarify this with my colleagues internally and I'll be back. |
tomako
force-pushed
the
introduce_layers_in_designer
branch
from
1357abc
to
8ffb9d6
Compare
And update on this? |
|
Sorry for the long silence. To be honest we are a bit reluctant to push some hidden feature useful only for us. In many ways it could cause headache in the future. So what if we just keep this here for a while until we see clearer how to integrate it in a better way? |
|
Sounds good, I'm changing the PR to draft for the time being. |
This is about to introduce layers to Poster & Badge Designer.
How it works: