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.

Sign up for GitHub

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

Jump to bottom

Sidebar headless UI menu #16333

Merged
merged 33 commits into from
Sep 22, 2022
Merged

Sidebar headless UI menu #16333

merged 33 commits into from
Sep 22, 2022

Conversation

matter-q
Copy link
Contributor

@matter-q matter-q commented Sep 5, 2022 •
edited
Loading

What

Closes #12617

How

  • Removed styled-components
  • Removed dropdown popoup menu
  • Created a new dropdown menu based on a headless UI menu
  • Implemented keyboard support for dropdown menu

Figma: https://www.figma.com/file/AxqFXuHo2BoTY0QY12hxm1/00_02_WEB-APP-COMPONENTS?node-id=5%3A335

Loom

OSS:
https://www.loom.com/share/440c0bc59df64711b6dae14641b1c795
Cloud:
https://www.loom.com/share/b22f1870ac3f4990b1856d74ad253bfb
Keyboard interaction:
https://www.loom.com/share/e484c92d953648938ca67f52a8ddfbf9

@github-actions github-actions bot added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Sep 5, 2022
@matter-q matter-q marked this pull request as ready for review September 5, 2022 14:46
@matter-q matter-q requested a review from a team as a code owner September 5, 2022 14:46
@timroes
Copy link
Contributor

timroes commented Sep 5, 2022

For validation please make sure that the menu is completely usable via keyboard only and doesn't require mouse anymore to operate.

@matter-q matter-q marked this pull request as draft September 7, 2022 13:43
@matter-q
Added keyboard support
8e27b07
@matter-q
Removed styled-components
87c0335
@matter-q
Merge branch 'master' into mark/sidebar-headless-ui-menu
0e979cc 
# Conflicts:
#	airbyte-webapp/src/packages/cloud/views/layout/SideBar/SideBar.tsx
#	airbyte-webapp/src/views/layout/SideBar/SideBar.tsx
@matter-q matter-q marked this pull request as ready for review September 7, 2022 23:13
teallarson
teallarson reviewed Sep 17, 2022
View reviewed changes
airbyte-webapp/src/views/layout/SideBar/components/SidebarDropdownMenu.module.scss Show resolved Hide resolved
airbyte-webapp/src/packages/cloud/views/layout/SideBar/SideBar.module.scss Outdated
border: 0;
border-radius: 10px;
background-color: rgba(255, 255, 255, 20%);
margin-top: 13px;
Copy link
Contributor

Choose a reason for hiding this comment

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

++ I'd bet we can use one of the variables

airbyte-webapp/src/packages/cloud/views/layout/SideBar/SideBar.module.scss Outdated Show resolved Hide resolved
airbyte-webapp/src/views/layout/SideBar/components/SidebarDropdownMenu.module.scss Outdated Show resolved Hide resolved
@matter-q
Review fixes #6
d0b408e
@matter-q
Merge branch 'master' into mark/sidebar-headless-ui-menu
39b5977 
# Conflicts:
#	airbyte-webapp/src/views/Connection/CatalogTree/StreamHeader.tsx
@matter-q
Review fixes #7
906ae37
teallarson
teallarson approved these changes Sep 20, 2022
View reviewed changes
Copy link
Contributor

@teallarson teallarson left a comment

Choose a reason for hiding this comment

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

lgtm! looks like the branch needs to be updated.

@matter-q matter-q changed the title Mark/sidebar headless UI menu [WIP] Mark/sidebar headless UI menu Sep 20, 2022
@matter-q
Copy link
Contributor Author

@edmundito, @timroes

Regarding design, the menu should be next to the button and aligned with the button.

Currently, the menu is aligned on the bottom side of the button.
image

Figma:
image

Is it necessary to implement additional behavior (placement) for the menu?
It could be resolved in some ways:

  • We can try to implement our own logic
  • Use a specific plugin https://popper.js.org/ (not tested in our application). It's recommended by Headless UI

dizel852
dizel852 requested changes Sep 20, 2022
View reviewed changes
Copy link
Contributor

@dizel852 dizel852 left a comment

Choose a reason for hiding this comment

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

Overall - lgtm.
But need to fix the rest of css variables.
Also lets wait for reply regarding centering the dropdown relatively control button

airbyte-webapp/src/packages/cloud/views/layout/SideBar/SideBar.module.scss Outdated Show resolved Hide resolved
airbyte-webapp/src/packages/cloud/views/layout/SideBar/SideBar.module.scss Outdated Show resolved Hide resolved
@edmundito
Copy link
Contributor

@matter-q the webapp is already using https://floating-ui.com/ for the tooltips, which is what it's most recommended over popper.js now days. That can allow you to align the menus in the right place.

@matter-q matter-q changed the title [WIP] Mark/sidebar headless UI menu Mark/sidebar headless UI menu Sep 22, 2022
@matter-q matter-q changed the title Mark/sidebar headless UI menu Sidebar headless UI menu Sep 22, 2022
dizel852
dizel852 approved these changes Sep 22, 2022
View reviewed changes
Copy link
Contributor

@dizel852 dizel852 left a comment

Choose a reason for hiding this comment

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

All PR comments are fixed, tested locally previously

@matter-q matter-q merged commit e685883 into master Sep 22, 2022
@matter-q matter-q deleted the mark/sidebar-headless-ui-menu branch September 22, 2022 18:33
This was referenced Sep 22, 2022
Closed
Merged
robbinhan pushed a commit to robbinhan/airbyte that referenced this pull request Sep 29, 2022
@dizel852 @robbinhan
Sidebar headless UI menu (airbytehq#16333)
66d3b19 
* Replaced sidebar menu with headlessui

* Removed unnecessary tag li

* Removed unnecessary Sidebar Popout

* Changed export type

* Added keyboard support

* Removed styled-components

* Removed commented code

* Disabled ESLint rule css-modules/no-undef-class

* Review fixes

* Fixed shorthand property

* Review fixes

* Review fixes airbytehq#2

* Review fixes airbytehq#3

* Review fixes airbytehq#4

* Review fixes airbytehq#5

* Review fixes airbytehq#6

* Review fixes airbytehq#7

* Update airbyte-webapp/src/packages/cloud/views/layout/SideBar/SideBar.module.scss

Co-authored-by: Vladimir <[email protected]>

* Update airbyte-webapp/src/packages/cloud/views/layout/SideBar/SideBar.module.scss

Co-authored-by: Vladimir <[email protected]>

Co-authored-by: Vladimir <[email protected]>
jhammarstedt pushed a commit to jhammarstedt/airbyte that referenced this pull request Oct 31, 2022
@dizel852 @jhammarstedt
Sidebar headless UI menu (airbytehq#16333)
526127e 
* Replaced sidebar menu with headlessui

* Removed unnecessary tag li

* Removed unnecessary Sidebar Popout

* Changed export type

* Added keyboard support

* Removed styled-components

* Removed commented code

* Disabled ESLint rule css-modules/no-undef-class

* Review fixes

* Fixed shorthand property

* Review fixes

* Review fixes airbytehq#2

* Review fixes airbytehq#3

* Review fixes airbytehq#4

* Review fixes airbytehq#5

* Review fixes airbytehq#6

* Review fixes airbytehq#7

* Update airbyte-webapp/src/packages/cloud/views/layout/SideBar/SideBar.module.scss

Co-authored-by: Vladimir <[email protected]>

* Update airbyte-webapp/src/packages/cloud/views/layout/SideBar/SideBar.module.scss

Co-authored-by: Vladimir <[email protected]>

Co-authored-by: Vladimir <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edmundito edmundito edmundito left review comments

@timroes timroes timroes left review comments

@dizel852 dizel852 dizel852 approved these changes

@teallarson teallarson teallarson approved these changes

Assignees
No one assigned
Labels
area/frontend Related to the Airbyte webapp area/platform issues related to the platform
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PoC: Headless UI Menu
5 participants
@matter-q @timroes @edmundito @dizel852 @teallarson