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

added support for colors and styles for displayName in editor #390

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DesignDeveloperr
Copy link

image

@netlify
Copy link

netlify bot commented Dec 15, 2021

✔️ Deploy Preview for luckperms ready!

🔨 Explore the source changes: 5d1791e

🔍 Inspect the deploy log: https://app.netlify.com/sites/luckperms/deploys/61b9c904d74e4c0007f453c7

😎 Browse the preview: https://deploy-preview-390--luckperms.netlify.app/

@lucko
Copy link
Member

lucko commented Dec 31, 2021

Hi, thanks for this!

We had a chat in Discord about the changes, @Turbotailz had some concerns about using v-html, as you don't get any of the xss protections provided by Vue. Since we're dealing with user input in this case, I think it's a valid issue.

I think the best way to deal with the problem is to just use Vue components instead of separately generated HTML, what do you think?

@Turbotailz any other ideas?

@lucko
Copy link
Member

lucko commented Dec 31, 2021

Also looks like @Tobi406 made some progress here: #289

@Tobi406
Copy link
Contributor

Tobi406 commented Dec 31, 2021

I'd just like to note that my PR is related to #103, which also suggests coloring prefixes and suffixes, which is the problem with my implementation (figuring out how far we shall parse prefixes/suffixes).

Also: this PR does not support hex color codes, which I think would be an important feature. Or maybe I just haven't spotted it yet?

@Turbotailz
Copy link
Member

Yeah, while I appreciate the PR, I am not too comfortable with the amount of DOM manipulation being done, along with the usage of v-html. It would be great if we could somehow repurpose this to use Vue to handle the DOM stuff.

And as Tobi says, it doesn't support hex codes which seems to be all the rage these days. I can just see it now - all the people asking about support for that 😅 If we support original colour codes, we need to support hex as well.

@LoganDark
Copy link

I don't like this because it makes assumptions about how displayName will be interpreted. For example, I could have a plugin that interprets it as text component JSON, or even MiniMessage. Should LuckPerms support all of those, and if so, how would I select between them?

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.

5 participants