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.

Sign up for GitHub

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

Jump to bottom

Add upload fixed image in badge designer #5273

Merged
merged 4 commits into from
Mar 29, 2022
Merged

Add upload fixed image in badge designer #5273

merged 4 commits into from
Mar 29, 2022

Conversation

SegiNyn
Copy link
Contributor

@SegiNyn SegiNyn commented Mar 8, 2022

This adds an option to upload images specific to the badge from the badge designer.

The image can be added from the 'insert elements' dropdown list and is called Fixed image

Multiple Fixed images can be added to the badge.

@SegiNyn SegiNyn changed the base branch from master to 3.1.x March 8, 2022 11:21
@SegiNyn SegiNyn force-pushed the badge-designer-picture-field branch 2 times, most recently from 0a80515 to d319be8 Compare March 8, 2022 11:37
@SegiNyn SegiNyn marked this pull request as ready for review March 8, 2022 11:44
plourenco
plourenco reviewed Mar 9, 2022
View reviewed changes
Copy link
Member

@plourenco plourenco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! There were a few ESLint problems, but not worth reviewing those since we can refactor this file in a single commit before merging.

indico/modules/designer/client/js/index.js Outdated Show resolved Hide resolved
indico/modules/designer/controllers.py Outdated Show resolved Hide resolved
@plourenco
Copy link
Member

@ThiefMaster since there's currently more than one fixed element, what do you think about having both the FixedText and FixedImage as placeholders in the designer?
This could prevent client-side conditionals such as this.

@SegiNyn SegiNyn requested a review from plourenco March 11, 2022 09:42
@SegiNyn SegiNyn force-pushed the badge-designer-picture-field branch from 5e7f46f to dccfae9 Compare March 16, 2022 17:04
plourenco
plourenco approved these changes Mar 17, 2022
View reviewed changes
indico/modules/designer/client/js/index.js Outdated Show resolved Hide resolved
@SegiNyn
Copy link
Contributor Author

SegiNyn commented Mar 22, 2022

Hello, please can we merge this PR if it looks good, so we can include it in our next release

@ThiefMaster
Copy link
Member

Since you need to backport it anyway, I guess it'd be fine for you if we merged it into master and not 3.1.x?

@SegiNyn
Copy link
Contributor Author

SegiNyn commented Mar 22, 2022

Yes, it'll be fine

@SegiNyn SegiNyn changed the base branch from 3.1.x to master March 22, 2022 10:02
@ThiefMaster
Copy link
Member

Unless I'm blind we never check that the image_id when saving the badge actually belongs to that particular template... so you could actually "steal" an image from another event by simply sending a different ID...

@SegiNyn
Copy link
Contributor Author

SegiNyn commented Mar 22, 2022 •
edited
Loading

Yes you are correct, there is no check that the image_id belongs to the template. I can add a check for it.

@SegiNyn SegiNyn force-pushed the badge-designer-picture-field branch from d3ceeef to ae52d2b Compare March 23, 2022 14:57
ThiefMaster
ThiefMaster reviewed Mar 23, 2022
View reviewed changes
indico/modules/designer/controllers.py Outdated Show resolved Hide resolved
indico/modules/designer/controllers.py Outdated Show resolved Hide resolved
indico/modules/designer/controllers.py Outdated Show resolved Hide resolved
indico/modules/designer/controllers.py Outdated
new_template = DesignerTemplate(title=title, type=self.template.type, **self.target_dict)

data = self.template.data
image_items = [(i, item) for i, item in enumerate(data['items']) if item['type'] == 'fixed_image']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the enumerate is needed, there's no need to use an index. When you iterate over a list of items and mutate an item, it will change the item in the list.

indico/modules/designer/controllers.py Outdated Show resolved Hide resolved
indico/modules/designer/controllers.py Outdated Show resolved Hide resolved
indico/modules/designer/controllers.py Outdated Show resolved Hide resolved
indico/modules/designer/placeholders.py Outdated Show resolved Hide resolved
@SegiNyn SegiNyn force-pushed the badge-designer-picture-field branch 5 times, most recently from 4f33f92 to 0b273cf Compare March 29, 2022 08:07
ThiefMaster
ThiefMaster reviewed Mar 29, 2022
View reviewed changes
indico/modules/designer/controllers.py Outdated Show resolved Hide resolved
indico/modules/designer/controllers.py Outdated Show resolved Hide resolved
indico/modules/events/registration/badges.py Outdated Show resolved Hide resolved
@ThiefMaster ThiefMaster enabled auto-merge (squash) March 29, 2022 11:27
@ThiefMaster ThiefMaster merged commit e0f81e2 into indico:master Mar 29, 2022
@ThiefMaster ThiefMaster deleted the badge-designer-picture-field branch March 29, 2022 11:33
SegiNyn added a commit to UNOG-Indico/indico-core that referenced this pull request Mar 29, 2022
@SegiNyn
Add support for images in designer templates (indico#5273)
9e9642b
SegiNyn added a commit to UNOG-Indico/indico-core that referenced this pull request Mar 29, 2022
@SegiNyn
Add support for images in designer templates (indico#5273)
21f4d5c
SegiNyn added a commit to UNOG-Indico/indico-core that referenced this pull request Mar 30, 2022
@SegiNyn
Add support for images in designer templates (indico#5273)
98c5411
SegiNyn added a commit to UNOG-Indico/indico-core that referenced this pull request Mar 30, 2022
@SegiNyn
BACKPORT Add support for images in designer templates (indico#5273)
7cbf840
OmeGak pushed a commit to UNOG-Indico/indico-core that referenced this pull request Dec 8, 2022
@SegiNyn @OmeGak
Add support for images in designer templates (indico#5273)
0ce37fe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@plourenco plourenco plourenco approved these changes

@ThiefMaster ThiefMaster ThiefMaster approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SegiNyn @plourenco @ThiefMaster