-
Notifications
You must be signed in to change notification settings - Fork 92
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
Add a gcp project picker to the repo flow #5003
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.
I think you can simplify this by having the Picker render inline rather than in a modal, and then instead using <Modal>
from app/components/modal/modal.tsx
<Modal
isVisible={this.state.isPickerVisible}
onRequestClose={() => this.setState({ isPickerVisible: false })} >
<Picker
options={this.state.pickerOptions}
onSelect={selection => { this.setState({ selection, isPickerVisible: false}); } />
</Modal>
Benefits:
- Can get rid of the custom CSS for showing the backdrop behind the picker (Modal should already handle this)
- Inherit other nice behaviors from
<Modal>
such as dismissing with Esc Picker
becomes more generally useful, e.g. we might have a form where it makes sense to render this inline in the form- No need for the global picker service which adds a bit of complexity / indirection
- No need to render
<Picker>
in the root component
app/picker/picker.tsx
Outdated
selectedIndex: number; | ||
} | ||
|
||
export default class PickerComponent extends React.Component<{}, State> { |
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.
nit: the stuff under app/components doesn't use the "Component" suffix
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.
Done
I've moved the Picker component to use a I like having the picker service, because it allows you to just say "have the user pick one of these options and give me the result in a promise" rather than having to build a whole fancy dom structure and manage state any time you want the user to pick something. Kind of like the old |
Screenshot: