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

fix: add close button on mobile #119

Merged
merged 9 commits into from
Sep 13, 2023
Merged

fix: add close button on mobile #119

merged 9 commits into from
Sep 13, 2023

Conversation

Abhikumar98
Copy link
Contributor

CleanShot 2023-08-14 at 13 46 36@2x

Open to suggestions on the placement of the button 😂

Copy link
Member

@chybisov chybisov left a comment

Choose a reason for hiding this comment

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

Please also check how this works with split subvariant

image

packages/widget/src/AppDrawer.tsx Outdated Show resolved Hide resolved
packages/widget/src/AppDrawer.style.tsx Outdated Show resolved Hide resolved
@Abhikumar98 Abhikumar98 requested a review from chybisov September 4, 2023 10:04
@Abhikumar98
Copy link
Contributor Author

Please also check how this works with split subvariant

image

Yeah, this is causing an issue.
Can you suggest where it'd be best to keep the close button in this variant?

This works well for default and lifuel variant
CleanShot 2023-09-04 at 19 07 10@2x
CleanShot 2023-09-04 at 19 07 05@2x

But breaking for split
CleanShot 2023-09-04 at 19 07 19@2x

@Abhikumar98
Copy link
Contributor Author

I've pushed the icons to the left and accommodated some space for close button, this would happen only if the variant is drawer and subvariant is split

CleanShot 2023-09-04 at 19 31 17@2x CleanShot 2023-09-04 at 19 31 13@2x

@Abhikumar98 Abhikumar98 dismissed chybisov’s stale review September 4, 2023 10:55

Worked on the changes

Copy link
Member

@chybisov chybisov left a comment

Choose a reason for hiding this comment

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

@Abhikumar98 I like the idea. There are some small inaccuracies though

  1. Icons should be the same size, including hover effects.
image
  1. Default mode has unnecessary space on the right of the settings button.
image

@Abhikumar98
Copy link
Contributor Author

@Abhikumar98 I like the idea. There are some small inaccuracies though

  1. Icons should be the same size, including hover effects.
image 2. Default mode has unnecessary space on the right of the settings button. image

Pushed the changes 😄

@Abhikumar98 Abhikumar98 dismissed chybisov’s stale review September 6, 2023 13:51

all changes done

Copy link
Member

@chybisov chybisov left a comment

Choose a reason for hiding this comment

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

@Abhikumar98, please follow the edge 😅

Before

image

After

image image

@Abhikumar98
Copy link
Contributor Author

@Abhikumar98, please follow the edge 😅

Before

image **After**

image image

Hey @chybisov,
Missed this comment 🙇🏽

I've pushed the fixes though.
This was really a great notice, thanks for this review!!

@chybisov
Copy link
Member

chybisov commented Sep 13, 2023

@Abhikumar98, it seems like the fix made things worse. 😅 We should have equal indent between icons and also use edge setting or find the right padding/margin for the cross icon.

image image

@chybisov chybisov merged commit 8c2ab60 into main Sep 13, 2023
@chybisov chybisov deleted the lf-2592-close-button branch September 13, 2023 18:19
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