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

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

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

matter-q
Copy link
Contributor

@matter-q matter-q 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
@matter-q matter-q 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
@matter-q matter-q marked this pull request as ready for review October 7, 2022 12:31
@matter-q matter-q requested a review from a team as a code owner October 7, 2022 12:31
@matter-q matter-q 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?

@matter-q
Copy link
Contributor Author

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

@matter-q matter-q marked this pull request as draft October 17, 2022 12:35
@matter-q matter-q 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
@matter-q matter-q 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
@matter-q matter-q 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
@matter-q matter-q marked this pull request as ready for review November 2, 2022 18:35
@matter-q matter-q 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
@matter-q matter-q requested a review from teallarson November 4, 2022 20:32
@matter-q matter-q 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
@matter-q matter-q 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
@matter-q matter-q requested a review from krishnaglick November 8, 2022 12:13
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
@@ -54,16 +58,18 @@ const SourceItemPage: React.FC = () => {

const connectionsWithSource = connections.filter((connectionItem) => connectionItem.sourceId === source.sourceId);

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

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

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
@@ -59,20 +63,22 @@ const DestinationItemPage: React.FC = () => {
(connectionItem) => connectionItem.destinationId === destination.destinationId
);

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

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

Choose a reason for hiding this comment

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

Fixed. Thanks!

@matter-q matter-q 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
@matter-q matter-q 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.

@matter-q
Replaced react-select menu with headless ui menu
847d2bb 
- Code refactoring
- Added keyboard interaction
@matter-q
Replaced react-select menu with headless ui menu
d649aca 
- Code refactoring
- Added keyboard interaction
@matter-q
Replaced react-select menu with headless ui menu
e0e6794 
- Code refactoring
- Added keyboard interaction
@matter-q matter-q 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
@matter-q matter-q merged commit 79e8959 into master Nov 10, 2022
@matter-q matter-q 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
Reviewers

@krishnaglick krishnaglick krishnaglick left review comments

@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.

Replace React Select with Headless UI menu in the destination page add source button
3 participants
@matter-q @krishnaglick @teallarson