Skip to content

Replaced react-select menu with headless ui menu source and destination buttons #17664

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
Merged
merged 3 commits into from
Nov 10, 2022
Merged

Replaced react-select menu with headless ui menu source and destination buttons #17664

merged 3 commits into from
Nov 10, 2022

Conversation

@m19berger
Copy link
Contributor

@m19berger m19berger commented Oct 6, 2022 •
edited
Loading

What

Closes #17501

How

Replaced react-select menu with headless ui menu source and destination buttons

  • Code refactoring
  • Replaced react-selcet with headless ui
  • Added keyboard interaction

Loom

Updated Loom with latest changes
https://www.loom.com/share/16412d05a4fc497392bc124ae3b5ef5a
@github-actions github-actions bot added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Oct 6, 2022
@m19berger m19berger force-pushed the mark/replace-react-select-with-headless-ui-source-and-destination-button branch from d1d5b29 to fcc9c08 Compare October 6, 2022 12:34
@m19berger m19berger marked this pull request as ready for review October 7, 2022 12:31
@m19berger m19berger requested a review from a team as a code owner October 7, 2022 12:31
@m19berger m19berger force-pushed the mark/replace-react-select-with-headless-ui-source-and-destination-button branch from 0d19973 to e58c6e9 Compare October 7, 2022 20:58
teallarson
teallarson reviewed Oct 11, 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.

I see that we have SidebarDropdown and DropdownMenu now in our codebase and they repeat a lot of the same styles and functionality.

Could SidebarDropdown either be a DropdownMenu or utilize DropdownMenu to decrease the repeated code?
@m19berger
Copy link
Contributor Author

m19berger commented Oct 12, 2022

I see that we have SidebarDropdown and DropdownMenu now in our codebase and they repeat a lot of the same styles and functionality.

Could SidebarDropdown either be a DropdownMenu or utilize DropdownMenu to decrease the repeated code?

Good idea 😀
I need to find a possible way to make one component for this. There are some differences in behavior and interfaces
@m19berger m19berger marked this pull request as draft October 17, 2022 12:35
@m19berger m19berger changed the title Replaced react-select menu with headless ui menu source and destination buttons [WIP] Replaced react-select menu with headless ui menu source and destination buttons Oct 17, 2022
@edmundito edmundito mentioned this pull request Oct 24, 2022
Merged
@m19berger m19berger force-pushed the mark/replace-react-select-with-headless-ui-source-and-destination-button branch 2 times, most recently from 2c66591 to e715bfb Compare October 30, 2022 16:19
@m19berger m19berger changed the title [WIP] Replaced react-select menu with headless ui menu source and destination buttons Replaced react-select menu with headless ui menu source and destination buttons Nov 2, 2022
@m19berger m19berger marked this pull request as ready for review November 2, 2022 18:35
@m19berger m19berger force-pushed the mark/replace-react-select-with-headless-ui-source-and-destination-button branch 7 times, most recently from 9c86fe2 to 961ea4e Compare November 4, 2022 14:10
@m19berger m19berger requested a review from teallarson November 4, 2022 20:32
@m19berger m19berger force-pushed the mark/replace-react-select-with-headless-ui-source-and-destination-button branch from 961ea4e to 6caa5b7 Compare November 7, 2022 09:49
krishnaglick
krishnaglick reviewed Nov 7, 2022
View reviewed changes
@m19berger m19berger force-pushed the mark/replace-react-select-with-headless-ui-source-and-destination-button branch 2 times, most recently from d62198e to c1814a3 Compare November 8, 2022 01:03
teallarson
teallarson requested changes Nov 9, 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.

This looks a lot closer!

Instead of typing the individual properties on the options where we implement this, let's type the entire object. I left a comment where I saw this. After that, I'd say it looks good to me!
airbyte-webapp/src/pages/SourcesPage/pages/SourceItemPage/SourceItemPage.tsx Outdated
const connectionsWithSource = connections.filter((connectionItem) => connectionItem.sourceId === source.sourceId);

const destinationsDropDownData = useMemo(
const destinationDropdownOptions = useMemo(
Copy link
Contributor

@teallarson teallarson Nov 9, 2022

Choose a reason for hiding this comment

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

Suggested change
const destinationDropdownOptions = useMemo(
const destinationDropdownOptions: DropdownMenuOptionType[] = useMemo(

should enable us to remove the casting below ("button" as DropdownMenuElementType and others)
Copy link
Contributor Author

@m19berger m19berger Nov 10, 2022

Choose a reason for hiding this comment

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

Fixed. Thanks!
airbyte-webapp/src/pages/DestinationPage/pages/DestinationItemPage/DestinationItemPage.tsx Outdated
);

const sourcesDropDownData = useMemo(
const sourceDropdownOptions = useMemo(
Copy link
Contributor

@teallarson teallarson Nov 9, 2022

Choose a reason for hiding this comment

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

Suggested change
const sourceDropdownOptions = useMemo(
const sourceDropdownOptions: DropdownMenuOptionType[] = useMemo(

should enable us to remove the casting below (ie: "right" as DropdownMenuItemIconPositionType)
Copy link
Contributor Author

@m19berger m19berger Nov 10, 2022

Choose a reason for hiding this comment

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

Fixed. Thanks!
@m19berger m19berger force-pushed the mark/replace-react-select-with-headless-ui-source-and-destination-button branch from 267c486 to 42cb0e3 Compare November 10, 2022 13:29
@m19berger m19berger requested a review from teallarson November 10, 2022 13:31
@krishnaglick krishnaglick removed their request for review November 10, 2022 14:39
teallarson
teallarson approved these changes Nov 10, 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.

Looks good! Did not retest locally after the type change.
@m19berger
Replaced react-select menu with headless ui menu
847d2bb 
- Code refactoring - Added keyboard interaction
@m19berger
Replaced react-select menu with headless ui menu
d649aca 
- Code refactoring - Added keyboard interaction
@m19berger
Replaced react-select menu with headless ui menu
e0e6794 
- Code refactoring - Added keyboard interaction
@m19berger m19berger force-pushed the mark/replace-react-select-with-headless-ui-source-and-destination-button branch from 42cb0e3 to e0e6794 Compare November 10, 2022 21:30
@m19berger m19berger merged commit 79e8959 into master Nov 10, 2022
@m19berger m19berger deleted the mark/replace-react-select-with-headless-ui-source-and-destination-button branch November 10, 2022 22:03
@github-actions github-actions bot mentioned this pull request Nov 15, 2022
Closed
@octavia-squidington-iii octavia-squidington-iii mentioned this pull request Nov 18, 2022
Merged
akashkulk pushed a commit that referenced this pull request Dec 2, 2022
@akashkulk
Replaced react-select menu with headless ui menu source and `destin…
08ce908 
…ation` buttons (#17664) * Replaced react-select menu with headless ui menu - Code refactoring - Added keyboard interaction
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/frontend Related to the Airbyte webapp area/platform issues related to the platform

4 participants

@m19berger @krishnaglick @teallarson