Skip to content

fix: textarea ux in Editor.tsx #16225#16231

Merged
Udit-takkar merged 8 commits intocalcom:mainfrom
thefcraft:main
Aug 20, 2024
Merged

fix: textarea ux in Editor.tsx #16225#16231
Udit-takkar merged 8 commits intocalcom:mainfrom
thefcraft:main

Conversation

@thefcraft
Copy link
Copy Markdown
Contributor

@thefcraft thefcraft commented Aug 16, 2024 •
edited
Loading

What does this PR do?

also follows [pull request] 16226 changes

Screenshot 2024-08-16 144852

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • I have added a Docs issue here if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

i tested the ui my self
and run the test cases

Checklist

@vercel
Copy link
Copy Markdown

vercel Bot commented Aug 16, 2024

@thefcraft is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@graphite-app graphite-app Bot added the community Created by Linear-GitHub Sync label Aug 16, 2024
@graphite-app graphite-app Bot requested a review from a team August 16, 2024 09:21
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Aug 16, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions Bot added ui area: UI, frontend, button, form, input 🐛 bug Something isn't working labels Aug 16, 2024
@thefcraft thefcraft mentioned this pull request Aug 16, 2024
Closed
3 tasks
@graphite-app
Copy link
Copy Markdown

graphite-app Bot commented Aug 16, 2024

Graphite Automations

"Add consumer team as reviewer" took an action on this PR • (08/16/24)

1 reviewer was added to this PR based on Keith Williams's automation.

"Add community label" took an action on this PR • (08/16/24)

1 label was added to this PR based on Keith Williams's automation.

anikdhabal
anikdhabal reviewed Aug 16, 2024
View reviewed changes
Comment thread packages/ui/components/dialog/Dialog.tsx Outdated
anikdhabal
anikdhabal reviewed Aug 16, 2024
View reviewed changes
Comment thread packages/ui/components/editor/Editor.tsx Outdated
anikdhabal
anikdhabal requested changes Aug 16, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@anikdhabal anikdhabal left a comment

Choose a reason for hiding this comment

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

Your changes did not fix the actual issue; now it hides the ring color using hardcoded styles. I don't think this is a good approach.

The only thing that is needed, and I thought about it before, is a max-height property for the editor.

Amit91848
Amit91848 reviewed Aug 16, 2024
View reviewed changes
Comment thread packages/features/eventtypes/components/CreateEventTypeDialog.tsx Outdated
@thefcraft
Copy link
Copy Markdown
Contributor Author

thefcraft commented Aug 16, 2024

anikdhab

DialogFooter showDivider className="-ml-1 -mr-1 border-l-4 border-r-4" this line does

Screenshot 2024-08-16 152215

@thefcraft
Copy link
Copy Markdown
Contributor Author

thefcraft commented Aug 16, 2024 •
edited
Loading

Untitled.video.-.Made.with.Clipchamp.1.mp4

have a look, max height is for different purpose

Amit91848
Amit91848 previously requested changes Aug 16, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@Amit91848 Amit91848 left a comment

Choose a reason for hiding this comment

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

Unit tests are failing @thefcraft, please look into it. Also suggested some changes which would solve the problem for all the Dialogs

Comment thread packages/ui/components/editor/Editor.tsx Outdated
Comment thread packages/features/eventtypes/components/CreateEventTypeDialog.tsx Outdated
Comment thread packages/ui/components/dialog/Dialog.tsx Outdated
Comment thread packages/ui/components/dialog/Dialog.tsx
anikdhabal
anikdhabal requested changes Aug 16, 2024
View reviewed changes
Comment thread packages/ui/components/dialog/Dialog.tsx Outdated
<div
className={classNames(
"flex justify-end space-x-2 pb-4 pt-4 rtl:space-x-reverse",
"bg-default -ml-1 -mr-1",
Copy link
Copy Markdown
Contributor

@anikdhabal anikdhabal Aug 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's works, but definitely not pixel perfect. My question is, for one dialog or a simple issue, why do other dialogs suffer?

Screenshot 2024-08-16 193625

People often don't provide as much description when creating an event, which causes the editor to go off-screen. I suggest adding an optional max-height property to the editor to ensure it doesn't go off-screen. Maximum text editor has the ability to set max height, it would be useful in other cases also.

anikdhabal
anikdhabal approved these changes Aug 17, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@anikdhabal anikdhabal left a comment

Choose a reason for hiding this comment

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

LGTM!!

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Aug 17, 2024 •
edited
Loading

E2E results are ready!

@Udit-takkar Udit-takkar enabled auto-merge (squash) August 20, 2024 08:22
@Udit-takkar Udit-takkar merged commit b004587 into calcom:main Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anikdhabal anikdhabal anikdhabal approved these changes

@Amit91848 Amit91848 Awaiting requested review from Amit91848

Assignees

No one assigned

Labels

🐛 bug Something isn't working community Created by Linear-GitHub Sync ready-for-e2e ui area: UI, frontend, button, form, input

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

just a small ui bug

5 participants

@thefcraft @CLAassistant @Amit91848 @anikdhabal @Udit-takkar