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

Segment Control #5170

Merged
merged 5 commits into from Oct 25, 2022
Merged

Segment Control #5170

merged 5 commits into from Oct 25, 2022

Conversation

sean-brydon
Copy link
Member

@sean-brydon sean-brydon commented Oct 22, 2022

What does this PR do?

Fixes: #5151

Added example to sandbox I know this isn't really used but radix tailwind doesn't like storybook as it doesn't generate the styles.

@Jaibles Visit localhost:3000/sandbox/form or use the preview URL https://cal-git-segmentcontroll-cal.vercel.app/sandbox/form for the same thing :)

CleanShot 2022-10-22 at 13 50 21@2x

@vercel
Copy link

vercel bot commented Oct 22, 2022

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

Name Status Preview Updated
cal ✅ Ready (Inspect) Visit Preview Oct 25, 2022 at 10:44AM (UTC)

Copy link
Member

@PeerRich PeerRich left a comment

Choose a reason for hiding this comment

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

looks great

@JeroenReumkens
Copy link
Contributor

Oh that's interesting. I just stumbled on this issue, and I'm building a very similar toggle for the new am/pm toggle:

CleanShot 2022-10-24 at 13 24 54@2x

However, in my case semantically it shouldn't be a radio button but actual buttons, so I opted for the toggle group. Although in other place in the design a radio button definitely is the right choice. So I think that in the future we should see if we could somehow share the styles, but give the dev the option to use it either as a radio button or as a native button toggle. What do you think?

My WIP commit:
1ccecbc

@sean-brydon
Copy link
Member Author

Oh that's interesting. I just stumbled on this issue, and I'm building a very similar toggle for the new am/pm toggle:

CleanShot 2022-10-24 at 13 24 54@2x

However, in my case semantically it shouldn't be a radio button but actual buttons, so I opted for the toggle group. Although in other place in the design a radio button definitely is the right choice. So I think that in the future we should see if we could somehow share the styles, but give the dev the option to use it either as a radio button or as a native button toggle. What do you think?

My WIP commit: 1ccecbc

Although I agree with what you are saying - I believe the functionality of both is different. One is a form input and the other isn't ? that just doesn't sit right with me in some ways

@JeroenReumkens
Copy link
Contributor

Although I agree with what you are saying - I believe the functionality of both is different. One is a form input and the other isn't ? that just doesn't sit right with me in some ways

Yes agree. It is visually similar, but the implementation is different indeed. Not sure if we can merge it somehow, at least styling wise. But it should still be able to be a button or a radio. For now lets keep it in the back of our minds and see if we need to merge it one day. I did for example add an animation when switching the toggle, would be nice if those things stayed consistent.

@ciaranha
Copy link
Member

ciaranha commented Oct 25, 2022

@sean-brydon looks perfect

(@JeroenReumkens I had actually set Seán this task after designing that am/pm task thinking it could be reused so good to know 2 types are needed)

@sean-brydon sean-brydon added ♻️ autoupdate tells kodiak to keep this branch up-to-date automerge labels Oct 25, 2022
@kodiakhq kodiakhq bot merged commit 5f1d203 into main Oct 25, 2022
@kodiakhq kodiakhq bot deleted the segment_controll branch October 25, 2022 10:47
Udit-takkar pushed a commit that referenced this pull request Oct 26, 2022
* Segment

* Fix type errors

* Fix HUG
haffla pushed a commit to tourlane/cal.com that referenced this pull request Nov 22, 2022
* Segment

* Fix type errors

* Fix HUG
haffla pushed a commit to tourlane/cal.com that referenced this pull request Nov 22, 2022
* Segment

* Fix type errors

* Fix HUG
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge ♻️ autoupdate tells kodiak to keep this branch up-to-date
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CAL-220] Build Segment component
5 participants