Skip to content

#5131 -Event Type Settings Inconsistency #5147

Merged
zomars merged 10 commits intomainfrom
5131-cal-210-inconsistency-between-how-toggle-on-settings-appear-in-event-type-edit
Oct 24, 2022
Merged

#5131 -Event Type Settings Inconsistency #5147
zomars merged 10 commits intomainfrom
5131-cal-210-inconsistency-between-how-toggle-on-settings-appear-in-event-type-edit

Conversation

@sean-brydon
Copy link
Copy Markdown
Member

Fixes the settings inconsistency throughout the Event Type pages

https://www.loom.com/share/0f436a1c729242f984b1328ebd22c320

@linear
Copy link
Copy Markdown

linear Bot commented Oct 21, 2022

CAL-210 Inconsistency between how toggle on settings appear in event type edit

When you're editing an event type and toggle on any settings, the behaviour for how and where they appear is very inconsistent.

Some animate in and some just appear, some are disabled and then become enabled. Then in terms of position some are correctly aligned with the text above, and some are aligned to the far left. Then some are half way between.

The correct appearance is in Figma

↳ Event Type Single ✓ - ❖ Cal DS (Figma)

@vercel
Copy link
Copy Markdown

vercel Bot commented Oct 21, 2022

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

Name Status Preview Updated
cal ✅ Ready (Inspect) Visit Preview Oct 24, 2022 at 11:16PM (UTC)

</div>
</div>
{children && (
<div className="mt-4 lg:ml-14" ref={animateRef}>
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We render an empty div with the animation ref so we can slowly fade in the content if !checked

name="periodType"
control={formMethods.control}
render={({ field: { value } }) => (
<SettingsToggle
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Moves bulk of code into the component - children are passed through to be rendered on true

Comment thread packages/ui/package.json
"react": "^18.2.0",
"react-colorful": "^5.6.0",
"react-feather": "^2.0.10",
"@formkit/auto-animate": "^1.0.0-beta.1",
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Why was this not in UI already :D

@ciaranha
Copy link
Copy Markdown
Member

Looks like a great solution to me 👏

@ciaranha ciaranha added this to the v.2.2 milestone Oct 21, 2022
@ciaranha ciaranha added the 🧹 Improvements Improvements to existing features. Mostly UX/UI label Oct 21, 2022
@sean-brydon sean-brydon requested a review from a team October 22, 2022 12:19
<SettingsToggle
title={t("limit_booking_frequency")}
description={t("limit_booking_frequency_description")}
checked={Object.keys(value ?? {}).length > 0}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would enabled onEnabledChange - be better naming?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not sure here tbs - checked is also shared with out switch component so if we were to change it one place I'd deffo saw we should change it there too

@@ -0,0 +1,57 @@
import { useAutoAnimate } from "@formkit/auto-animate/react";
Copy link
Copy Markdown
Contributor

@emrysal emrysal Oct 24, 2022

Choose a reason for hiding this comment

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

Does this have to be in v2 (this file I mean)?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No it doesn't - but I believe it should live here until we fully migrate everything from V2 -> v1 files so we don't accidentally remove anything we are meant to in the clean up

Copy link
Copy Markdown
Contributor

@zomars zomars left a comment

Choose a reason for hiding this comment

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

LGTM

@zomars zomars merged commit b18eabc into main Oct 24, 2022
@zomars zomars deleted the 5131-cal-210-inconsistency-between-how-toggle-on-settings-appear-in-event-type-edit branch October 24, 2022 23:11
Udit-takkar pushed a commit that referenced this pull request Oct 26, 2022
* Reusable component

* Fix limits not being toggleable

* Remove custom input margin

* addTestId

* Limits+adv

* Reccuring Tab

* Remove console .log

Co-authored-by: Omar López <zomars@me.com>
haffla pushed a commit to tourlane/cal.com that referenced this pull request Nov 22, 2022
* Reusable component

* Fix limits not being toggleable

* Remove custom input margin

* addTestId

* Limits+adv

* Reccuring Tab

* Remove console .log

Co-authored-by: Omar López <zomars@me.com>
haffla pushed a commit to tourlane/cal.com that referenced this pull request Nov 22, 2022
* Reusable component

* Fix limits not being toggleable

* Remove custom input margin

* addTestId

* Limits+adv

* Reccuring Tab

* Remove console .log

Co-authored-by: Omar López <zomars@me.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🧹 Improvements Improvements to existing features. Mostly UX/UI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CAL-210] Inconsistency between how toggle on settings appear in event type edit

4 participants