Skip to content
This repository was archived by the owner on Aug 15, 2019. It is now read-only.

Add a way to modify the message placeholder text #3

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

Add a way to modify the message placeholder text #3

wants to merge 1 commit into from

Conversation

rsenk330
Copy link

When calling replacePlaceholdersInMessage(), it now will first look for a data attribute on the field, data-validator-placeholder, before falling back to the original name.

This allows for something other than the field name to be displayed in error messages.

When calling `replacePlaceholdersInMessage()`, it now will
first look for a data attribute on the field
(`data-validator-placeholder`) before falling back to the original
`name`.
@jackfranklin
Copy link
Owner

Thanks for this! I think this brings about a potentially larger discussion. I've been thinking for a while about allowing more configuration to be set through data-foo type attributes so it's easier to customise. Would love to hear people's thoughts about using data attributes more vs configuring through JS.

@rsenk330
Copy link
Author

@jackfranklin I had originally planned on adding it directly in the JS, but decided against it mostly for the added ease of using a template engine to fill the data-attribute in on the server side.

However, I feel that only allowing configuration through data attributes could quickly make the HTML messy. Maybe a hybrid would be best (like Twitter Bootstrap's JS stuff), but this adds some extra complexity. At least it would allow people to choose what is best for them.

@jackfranklin
Copy link
Owner

@rsenk330 you share the same concerns as me. I like this approach but my concern is that if we do this for this one setting it's a bit inconsistent.

I think we either make more use of data attributes or implement this change (I like the idea behind this PR) but setting the value via JS.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants