style date pickers #876
style date pickers #876
Conversation
brandonreid
requested review from
pleek91,
znicholasbrown and
zhen0
as code owners
brandonreid
force-pushed
the
context-nav-components
branch
from
63df2c1
to
6e42659
Compare
brandonreid
force-pushed
the
date-pickers
branch
from
a27b222
to
a6f8387
Compare
| @@ -33,6 +33,7 @@ | |||
| const props = defineProps<{ | |||
| primary?: boolean, | |||
| flat?: boolean, | |||
| selected?: boolean, | |||
There was a problem hiding this comment.
What's selected in relation to a button? i believe this was moved from the button group component but I don't think it makes sense here. There's no model value on this component so all this does is provide alternate styling. I'd move this back to the button group.
There was a problem hiding this comment.
A number of additional components in the date picker also utilize a selected style, so at that point I opted to centralize these styles in the button. In the context of a small module like this, it felt reasonably to allow for selected style buttons:
Screen.Recording.2023-08-07.at.11.50.55.AM.mov
So if keeping it, I should surface a model value for selected so it can be used like <p-button v-model:selected="condition" />, huh?
There was a problem hiding this comment.
I don't think you need to surface a v-model for it since the button doesn't change that value (so there's never a need to emit the value back to the parent) - selected isn't a typical button state. Perhaps active as a boolean attribute is more closely in line with how you intend to use this and would give you the opportunity to merge the logic here with what already exists
There was a problem hiding this comment.
Hmm. So I had this prop named active, but changed it to selected just to be in alignment with the token name. The token name is selected > active since active is used as a token modifier for active states on things.
The experience of the button in this state does consistently constitute a selected button option in a group of others, just in the case of the date picker, there's lots of options spread out in a different layout than the ButtonGroup component.
Thoughts?
There was a problem hiding this comment.
I suppose there's no harm in naming the prop active or activated (which might be what I actually had before, to distinguish from the active/pressed button state). I might reach out for a huddle to think through this one. I'm not sure I fallow what you mean by "would give you the opportunity to merge the logic here with what already exists" as well.
There was a problem hiding this comment.
Hm ok I think this is sort of muddying the waters of responsibility for this component but short of changing how you intend to use it I think this is fine. I'd prefer that button groups (which aren't semantic elements afaik) would instead be styled radio buttons or tablist elements and wouldn't actually be wrapping the PButton component. Since selected doesn't add any accessibility attributes it's probably ok
There was a problem hiding this comment.
That's a great point on button groups actually.. I agree on the sentiment as well. To sleep well tonight I made a ticket for addressing bucket groups. Will push on for now.
brandonreid
force-pushed
the
date-pickers
branch
from
a0bc200
to
2468f2d
Compare
✅ Deploy Preview for prefect-design ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Styles the DateInput, DatePicker, and DateRangeInput components. I also hit up some buttons I missed previously as my find-all was for
<p-buttonbut there were a number of<PButtoninstances out there.