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

Allow label, helpText, prefix and suffix as params #154

Closed
wants to merge 5 commits into from

Conversation

tarwin
Copy link

@tarwin tarwin commented Jul 11, 2022

Allow label, helpText, prefix and suffix as params on components that have them as slots. The slots override the params.

Copy link
Member

@juzser juzser left a comment

Choose a reason for hiding this comment

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

I cannot approve this PR, it should be improved a bit.
But I think it's very close. If you need, I can update these features from my side.

slot(name="prefix")
template(#suffix, v-if="slots.suffix")
slot(name="suffix")
template(#help-text, v-if="slots['help-text']" || helpText)
Copy link
Member

Choose a reason for hiding this comment

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

Mistake

slot(name="help-text")
template(#label, v-if="slots.label || label")
slot(name="label" v-if="slots.label")
template(v-else-if="label") {{ label }}
Copy link
Member

Choose a reason for hiding this comment

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

I think we just need v-else here

@@ -78,6 +85,8 @@ Labelled(
v-else,
:is="multiline ? 'textarea' : 'input'",
:id="id",
:label="label",
Copy link
Member

Choose a reason for hiding this comment

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

Nah, why do we need this prop here?

@tarwin
Copy link
Author

tarwin commented Jul 12, 2022

It's likely that I'm screwing up because I still don't quite understand pug - the rules coming over from HTML are a little ... different? Or I just don't read it as well. Yes, please make those small changes if you would, sorry.

@juzser
Copy link
Member

juzser commented Jul 12, 2022

No worry, you did a good job, just small tweaks needed. I'm updating this feature together with migrating Polaris v9.20.0.

@tarwin
Copy link
Author

tarwin commented Jul 12, 2022

I know it's hard writing an open source project. You want people to be involved so you don't have to do it all yourself but you also want very specific standards :D

@juzser
Copy link
Member

juzser commented Jul 12, 2022

#155 solved this

@juzser juzser closed this Jul 12, 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.

2 participants