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

Add forward, backward and background color to badge designer #5139

Merged
merged 7 commits into from
Jan 10, 2022

Conversation

SegiNyn
Copy link
Contributor

@SegiNyn SegiNyn commented Nov 3, 2021

No description provided.

@SegiNyn SegiNyn changed the title Wip/z index Add forward, backward and background color to badge designer Nov 3, 2021
@SegiNyn SegiNyn changed the base branch from master to 3.1.x December 8, 2021 11:14
CHANGES.rst Outdated Show resolved Hide resolved
indico/modules/designer/client/js/index.js Outdated Show resolved Hide resolved
indico/modules/designer/pdf.py Outdated Show resolved Hide resolved
indico/modules/designer/templates/template.html Outdated Show resolved Hide resolved
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.

Added some minor suggestions but it works great, thanks! 🙌

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

pferreir commented Dec 9, 2021

The PR looks great, thanks!
Just a bit of a UX question: we are using a fixed list of colors for the foreground. Shouldn't we have the same interface for both colors?

@SegiNyn
Copy link
Contributor Author

SegiNyn commented Dec 9, 2021

Hi, I initially used the same fixed list of colours for the background but it was suggested in an initial review that using a color picker would be preferred

@pferreir
Copy link
Member

pferreir commented Dec 9, 2021

@SegiNyn sure. I'm just wondering whether we shouldn't apply the same to the bg color.

@ThiefMaster
Copy link
Member

Consistency would clearly be useful (when making the initial suggestion I probably didn't even realized that we already have a color selector there for something)

@SegiNyn
Copy link
Contributor Author

SegiNyn commented Dec 9, 2021

Yes it would be better to keep it consistent. I'll change the colour picker to the fixed list of colours.

@pferreir
Copy link
Member

pferreir commented Dec 9, 2021

@SegiNyn Sorry, my bad, what I meant was changing the fixed list (fg color) to a picker.

@SegiNyn
Copy link
Contributor Author

SegiNyn commented Dec 9, 2021

Okay, got it. Thanks!

Copy link
Member

@pferreir pferreir left a comment

Choose a reason for hiding this comment

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

Thanks once again. Just some minor comments/suggestions.

indico/modules/designer/client/js/index.js Outdated Show resolved Hide resolved
indico/modules/designer/client/js/index.js Outdated Show resolved Hide resolved
indico/modules/designer/client/js/index.js Outdated Show resolved Hide resolved
indico/modules/events/registration/badges.py Outdated Show resolved Hide resolved
indico/modules/designer/client/js/index.js Outdated Show resolved Hide resolved
@SegiNyn SegiNyn force-pushed the wip/z-index branch 2 times, most recently from 6c69d89 to 286cd46 Compare December 9, 2021 12:41
@pferreir
Copy link
Member

pferreir commented Dec 10, 2021

OK, thanks a lot. I think we're good code-wise, but I've found a few issues testing it:

e.g.

image

which actually then renders OK, but the preview is weird.

Then, there is an issue with the z-index, probably some side-effect of the algorithm we didn't foresee:

Kapture 2021-12-10 at 13 34 57

Finally, it's currently impossible to figure out when an item is selected, if it's got a background. I suggest we move from changing the background of the item to adding a thick border around, or something like that.

@SegiNyn
Copy link
Contributor Author

SegiNyn commented Dec 10, 2021

Hi, "I suggest we move from changing the background of the item to adding a thick border around, or something like that." So whenever an item is selected, we add a thick border around it so it is more noticeable which item was selected while still maintaining the background colour? or the background colour is removed and remains only a thick border when the item is selected?

@pferreir
Copy link
Member

So whenever an item is selected, we add a thick border around it so it is more noticeable which item was selected while still maintaining the background colour?

Yes, precisely!

@SegiNyn
Copy link
Contributor Author

SegiNyn commented Dec 14, 2021

Hello @pferreir, I updated the code by adding a thicker border to selected items, make the item height use its initial value so the text is not larger than the background and updated the html zIndex values in the normalizeZIndex function

@SegiNyn
Copy link
Contributor Author

SegiNyn commented Dec 14, 2021

I added a 'data-id' attribute for the divs so they can be easily selected using:
$('.ui-draggable[data-id=' + item.id + ']')
but I'm not sure if you would want this attribute. We could also use:
$('.ui-draggable').each(function(){ if($(this).data('id') === item.id){ $(this).css('zIndex', n + 10); } });

@SegiNyn
Copy link
Contributor Author

SegiNyn commented Dec 15, 2021

Hello @pferreir, please could you re-review the changes (I used the github re-review button at the top but the review status was not changing). Thank you

@ThiefMaster
Copy link
Member

I think we need to find a solution for this... at least going back to transparent background is needed for sure. Not sure what the best solution is though... I'll ping my colleagues in case anyone has a good idea.

(it's pretty ridiculous that the native color picker doesn't provide something for this!)

@SegiNyn
Copy link
Contributor Author

SegiNyn commented Jan 6, 2022

Okay, thank you. We also saw that Indico has a ColorPickerWidget and I'm testing to see if this would work inplace of the native color picker.

@ThiefMaster
Copy link
Member

That might indeed by worth looking into. I don't think it supports clearing either but adding it there is probably easier than adding a fully new widget wrapping the native one.

@vasantvohra vasantvohra force-pushed the wip/z-index branch 2 times, most recently from 681975d to 6552ea9 Compare January 6, 2022 12:59
@SegiNyn
Copy link
Contributor Author

SegiNyn commented Jan 6, 2022

Hello @ThiefMaster, we have updated the code to use the ColorPickerWidget, which has the option to clear the color, but this changes the look because of the extra input field
Screenshot 2022-01-06 at 14 59 12
Screenshot 2022-01-06 at 14 58 44

@ThiefMaster
Copy link
Member

Fine for me, I'd just apply some CSS so the whole color code fits in the text field and the color box has the same size as the field (and is maybe attached right to its left)

@SegiNyn
Copy link
Contributor Author

SegiNyn commented Jan 7, 2022

It now looks like this:
Screenshot 2022-01-07 at 16 58 09

@ThiefMaster
Copy link
Member

Much nicer. Could you "equalize" the heights and remove the spacing between the color display and the text field?

@SegiNyn
Copy link
Contributor Author

SegiNyn commented Jan 7, 2022

Screenshot 2022-01-07 at 17 36 21

@ThiefMaster
Copy link
Member

almost :) there are still rounded top-left/bottom-left corners on the text fields, but since that's no longer an outer border these corners should be regular now

@SegiNyn
Copy link
Contributor Author

SegiNyn commented Jan 7, 2022

Screenshot 2022-01-07 at 18 05 29

CHANGES.rst Outdated Show resolved Hide resolved
Co-authored-by: Adrian <adrian@planetcoding.net>
indico/modules/designer/client/js/index.js Outdated Show resolved Hide resolved
indico/modules/designer/client/js/index.js Outdated Show resolved Hide resolved
indico/modules/designer/client/js/index.js Outdated Show resolved Hide resolved
indico/modules/designer/client/js/index.js Outdated Show resolved Hide resolved
indico/modules/designer/templates/template.html Outdated Show resolved Hide resolved
@SegiNyn SegiNyn requested review from ThiefMaster and removed request for pferreir January 10, 2022 12:13
@ThiefMaster ThiefMaster merged commit a857208 into indico:3.1.x Jan 10, 2022
@ThiefMaster ThiefMaster deleted the wip/z-index branch January 10, 2022 14:26
SegiNyn added a commit to UNOG-Indico/indico-core that referenced this pull request Jan 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a z-index field in Poster and Badge Designer
5 participants