-
Notifications
You must be signed in to change notification settings - Fork 4.6k
🪟 [Multi Cloud] Geography Dropdown Component #18543
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
airbyte-webapp/src/components/GeographyDropdown/GeographyDropdown.module.scss
Outdated
Show resolved
Hide resolved
airbyte-webapp/src/components/GeographyDropdown/GeographyDropdown.module.scss
Show resolved
Hide resolved
airbyte-webapp/src/components/GeographyDropdown/GeographyDropdown.tsx
Outdated
Show resolved
Hide resolved
airbyte-webapp/src/components/GeographyDropdown/GeographyDropdown.tsx
Outdated
Show resolved
Hide resolved
airbyte-webapp/src/components/GeographyDropdown/GeographyDropdown.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few more comments, let me know if anything is unclear!
airbyte-webapp/src/components/GeographyDropdown/GeographyDropdown.module.scss
Outdated
Show resolved
Hide resolved
airbyte-webapp/src/components/GeographyDropdown/GeographyDropdown.module.scss
Outdated
Show resolved
Hide resolved
airbyte-webapp/src/components/GeographyDropdown/GeographyDropdown.tsx
Outdated
Show resolved
Hide resolved
airbyte-webapp/src/components/GeographyDropdown/GeographyDropdown.tsx
Outdated
Show resolved
Hide resolved
airbyte-webapp/src/components/GeographyDropdown/GeographyDropdown.tsx
Outdated
Show resolved
Hide resolved
airbyte-webapp/src/components/GeographyDropdown/GeographyDropdown.tsx
Outdated
Show resolved
Hide resolved
airbyte-webapp/src/components/GeographyDropdown/GeographyDropdown.tsx
Outdated
Show resolved
Hide resolved
@josephkmh thank you for the review! I apologize for missing silly details, I was struggling with types and trying to get this ready ASAP - will make sure to be more attentive next time!! I am still looking into how to set the |
airbyte-webapp/src/components/GeographyDropdown/GeographyDropdown.tsx
Outdated
Show resolved
Hide resolved
airbyte-webapp/src/components/GeographyDropdown/GeographyDropdown.tsx
Outdated
Show resolved
Hide resolved
airbyte-webapp/src/components/GeographyDropdown/GeographyDropdown.tsx
Outdated
Show resolved
Hide resolved
airbyte-webapp/src/components/GeographyDropdown/GeographyDropdown.tsx
Outdated
Show resolved
Hide resolved
|
||
const customStyles: StylesConfig<GeographySelectOption> = { | ||
option: (base, state) => { | ||
let backgroundColor = theme.white; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The theme
files are going to go away, since they are only used for our legacy styled-components system. We should not use them in new code anymore. Instead if you need to get access in TS to colors, you should explicitally export them from your own SCSS file, e.g. see https://github.com/airbytehq/airbyte/blob/master/airbyte-webapp/src/components/ui/StatusIcon/CircleLoader.module.scss#L21-L25
Ideally though whenever possible you should prefer setting className
instead and have all styling in the SCSS file (but there are legit cases when that's not necessarily possible, I'm not sure if this place here might support className
actually?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I advised to import from scss/export.module.scss
here, but seems like re-exporting is the preferred method. Sorry for the bad advice @natalyjazzviolin!
FWIW I think you could do all this styling in GeographyDropdown.module.scss
. You can beat react-select
's CSS specificity by using something like :global(.reactSelect__option--is-focused).option { ... }
. Note that :global()
is special CSS modules syntax that lets you reference a non-scoped "normal" CSS class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@josephkmh react-select
's specificity was trumping the GeographyDropdown.module.scss
styles, but this is really helpful, thank you! I'll get right on it!
@natalyjazzviolin could we move this component to |
CLosing this for now, since we first want to continue work on |
What
Closes https://github.com/airbytehq/airbyte-cloud/issues/3283
Recommended reading order
airbyte-webapp/src/components/GeographyDropdown/GeographyDropdown.tsx
airbyte-webapp/src/components/GeographyDropdown/GeographyDropdown.module.scss
airbyte-webapp/src/components/GeographyDropdown/index.stories.tsx
🚨 User Impact 🚨
Pre-merge Checklist
Expand the relevant checklist and delete the others.
New Connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampledocs/integrations/README.md
airbyte-integrations/builds.md
Airbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described hereUpdating a connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described hereConnector Generator
-scaffold
in their name) have been updated with the latest scaffold by running./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates
then checking in your changesTests
Unit
Put your unit tests output here.
Integration
Put your integration tests output here.
Acceptance
Put your acceptance tests output here.