-
Notifications
You must be signed in to change notification settings - Fork 49
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 custom color campcollaboration #5281
Add custom color campcollaboration #5281
Conversation
d5bd715
to
dd0258d
Compare
dd0258d
to
26ecabb
Compare
aae8e88
to
a9502ee
Compare
a9502ee
to
555f40b
Compare
⛔ Feature branch deployment currently inactive.If the PR is still open, you can add the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool 🚀
Core Meeting DecisionImplement as request by @usu |
38c0287
to
a63c516
Compare
a63c516
to
792a8db
Compare
1a20280
to
ae4ac45
Compare
#[ORM\Column(type: 'text', nullable: true)] | ||
public ?string $collaborationAcceptedBy = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this removed? Is was not used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was fully dead code. Unused, and not even tested.
['1', true], | ||
['12', true], | ||
['123', false], | ||
['🧑🏼🔧', true], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does one of these tests check for combining emojis? Such as skin color combiners etc.. I'm not an expert on this, so I don't know whether one of your tests already accounts for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already one that is made out of 3 combined emojis.
The Man Mechanic: Light Skin Tone emoji is a ZWJ sequence combining 👨 Man, 🏻 Light Skin Tone, Zero Width Joiner and 🔧 Wrench.
– Emojipedia
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's also the reason, that I've included some basic smileys below that should be each counted as one.
|
||
export default (i18n) => ({ | ||
validate: (value) => { | ||
return /\p{Extended_Pictographic}/u.test(value) ? size(value) <= 1 : value.length <= 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, I learned something today.
In https://stackoverflow.com/a/72727900 it says that extended pictographic does not match a lone snowflake emoji, is that really the case? Should we use (\p{Emoji}\uFE0F|\p{Emoji_Presentation})
as that post suggests? Or is that covered in your additional logic with the length?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is covered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks! As a future extension, I would like to be able to always change the color and abbreviation of my own camp collaboration, even if I'm only a guest. Would you agree?
@carlobeltrame It somehow doesn't work with clientprint 😢 I didn't test it with a composed emoji until now. With "normal" emojis it works. |
@carlobeltrame I think we may rather add a color and abbreviation on the user, instead of user editable camp collaborators. |
I see, but it looks like composed emoji are a general problem in client print. No reason to delay merging this feature in my opinion. |
@carlobeltrame indeed, emojis are very slow with clientprint 😢. It doesn't break, but initially it needs to load all emojis, and this is probably taking a while. |
That would work as well (though not as flexible). But if that's the ultimate goal, why didn't you implement that in the first place? |
If the loading time is the biggest problem, we could preload the emojis as soon as you navigate to a page with a print button. But also, currently, the combined emoji are rendered split up into their components. I think that is actually an intended fallback in the unicode standard, so it's okay but just not optimal. |
Because it serves another purpose. I wanted a way to distinguish different users in a camp. The user color/abbreviation would just be an override between generated and campcollaboration color/abbreviation, and be a better fallback. |
Followup PR after #5174