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

Fix - Prevent tag format without uppercase #1509

Draft
wants to merge 2 commits into
base: 1.x
Choose a base branch
from

Conversation

lguichard
Copy link
Contributor

No description provided.

Copy link

vercel bot commented Jan 28, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lunar-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 29, 2024 9:14pm

Copy link
Collaborator

@alecritson alecritson left a comment

Choose a reason for hiding this comment

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

Thanks @lguichard

Had a quick look and it didn't change to uppercase when editing in a resource that uses tags i.e. products

Do we need some sort of unique validation on this?

I'm not sure it's the best UX having the value changed as you type, unless there was a debounce, personally I think converting to uppercase on save is fine as long as the unique validation takes into account it will be uppercase.

@lguichard
Copy link
Contributor Author

Thanks for the feedback.

You're be right for UX. We need to change several places in panel to unified uppercase on tags. I'll review the code of tags components on this PR, give me some time.

"Do we need some sort of unique validation on this?" They must be unique

@lguichard
Copy link
Contributor Author

@alecritson I removed live() even debounce because there was some Filament bugs with unique validation.

This PR fix tag edition from sidebar menu not on tags associated to resource. In this case, i used TagInput by Filament but we don't have any option to custom render view after had created a tag. I plan to submit a PR at Filament in order to add new option.

@alecritson
Copy link
Collaborator

@lguichard I've converted to draft whilst this is being worked on, as soon as it's ready mark it as ready and I'll have a look

@glennjacobs glennjacobs added this to the v1.0 milestone Jan 31, 2024
@glennjacobs
Copy link
Contributor

I think forcing uppercase should be an optional behaviour, as it's a personal preference thing.

@wychoong
Copy link
Contributor

Does Laravel validation takes care of column collation?

@glennjacobs
Copy link
Contributor

@alecritson @lguichard I'm trying to understand what problem this is trying to fix, as we have no tests or description on this issue.

Is this a preference thing, or is there a problem? E.g. are we currently allowing both HotProduct and hotproduct for example?

@glennjacobs glennjacobs self-assigned this May 22, 2024
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.

4 participants