Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

svelte: Add bottom panel close button#63128

Merged
fkling merged 2 commits into
mainfrom
fkling/srch-450-ui-feedback-confusing-lack-of-x-to-close-references-pane
Jun 7, 2024
Merged

svelte: Add bottom panel close button#63128
fkling merged 2 commits into
mainfrom
fkling/srch-450-ui-feedback-confusing-lack-of-x-to-close-references-pane

Conversation

@fkling

@fkling fkling commented Jun 6, 2024

Copy link
Copy Markdown
Contributor

Closes SRCH-450

This commit adds a button to the right hand side of the tabs that allows closing the bottom panel.

For this I

  • extended the Tabs.svelte component to allow rendering something to the right hand side of the tabs.
  • added a new text variant to button that renders an unstyled button and shows a background on hover

Note that the icon as shown in the figma design doesn't exist in lucide.

2024-06-06_17-36

Test plan

Visual/manual inspection in light and dark mode

Changelog

  • Added bottom panel close button
  • Added 'text' variant to Button component
  • Added 'actions' slot to Tabs component
  • Added Button component stories

This commit adds a button to the right hand side of the tabs that
allows closing the bottom panel.

For this I

- extended the Tabs.svelte component to allow rendering something to the
  right hand side of the tabs.
- added a new `text` variant to button that renders an unstyled button
  and shows a background on hover

Note that the icon as shown in the figma design doesn't exist in lucide.
@fkling fkling requested a review from a team June 6, 2024 15:44
@fkling fkling self-assigned this Jun 6, 2024
@cla-bot cla-bot Bot added the cla-signed label Jun 6, 2024
'merged',
'link',
'icon',
'text',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh nice. I've wanted something like this in the past

&:global(.disabled),
&:disabled,
&[aria-disabled='true'] {
cursor: not-allowed;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why get rid of the not-allowed cursor?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Huh. Looks like it's working as expected still in storybook

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I wanted to leave some comments but didn't have time. I removed this selected because it's not used by us.

@taiyab

taiyab commented Jun 6, 2024

Copy link
Copy Markdown
Contributor

The icon is too small for that particular one to work well, let's try https://lucide.dev/icons/arrow-down-from-line instead.

Comment on lines +71 to +73
<div class="actions">
<slot name="header-actions" />
</div>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we use this slot for the last commit info when closed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We could though I don't know whether that works together with the commit message text-overflow stuff. Something to investigate.

@fkling

fkling commented Jun 6, 2024

Copy link
Copy Markdown
Contributor Author

@taiyab Should we make the button larger? It's currently small. But that will also increase the font size and it seemed a little too large to me.

@taiyab

taiyab commented Jun 6, 2024

Copy link
Copy Markdown
Contributor

@taiyab Should we make the button larger? It's currently small. But that will also increase the font size and it seemed a little too large to me.

Nope, no need for now. I'm designing a whole new design system and component library that will take care of this problem properly down the road.

@fkling fkling merged commit b2f1746 into main Jun 7, 2024
@fkling fkling deleted the fkling/srch-450-ui-feedback-confusing-lack-of-x-to-close-references-pane branch June 7, 2024 07:07
fkling added a commit that referenced this pull request Jun 11, 2024
Closes SRCH-496

#63128 added a dedicated button for closing the bottom panel but
clicking the button didn't cause the diff view to close. This commit
fixes that.

## Test plan

Manual testing.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants