-
Notifications
You must be signed in to change notification settings - Fork 14
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
feat(TypePicker): create TypePicker and other needed components #390
Conversation
added components: - overlay/Overlay: used as a pop over, will call `onCancel` if you click outside fo the div, or click the 'x'. The `navigation` prop is optional. Any `children` will overflow-y scroll. - item/DataType.tsx: defines the `DataTypes` type. Wraps the type name, description, and icon. `type` and `description`, are defined and passed down. May want to refactor to keep description and type defined here, and just use the type to determine all content. - nav/TabPicker: has size options 'sm' and 'md', and color options 'light' and 'dark' - overlay/TypePickerOverlay: a version of `Overlay` that lists the DataTypes and passes through the `navigation` component. `typesAndDescriptions` are defined here. - structure/ColumnType: the component used to show the column type with its icon. Is not 'clickable' if there is no 'onClick' func passed down. I a column has more than one type, it will indicate 'multi' - structure/TypePicker: takes all these components and creates a TypePicker. Click on the columnType to get a TypePickerOverlay popup. Choose between single or multi mode. The chosen type is kept in the hidden input named for the column (`${column-name}-type`) This commit also created scss files based on the folder structure: - chrome - item - nav - overlay - structure - type It also adds a story for each component under the `structure` tab
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.
LGTM, just a few things to fix.
|
||
window.addEventListener('click', isInOverlay) | ||
return ( | ||
window.addEventListener('click', isInOverlay) |
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.
should this be return function() { ... window.removeEventListener ...}
to cleanup?
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.
yes thank you... how... was this working?
if (Array.isArray(type)) { | ||
if (type.length > 1) { | ||
shownType = 'multi' | ||
} else { |
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.
Should have a branch for type.length == 0
, just to avoid the undefined case.
stories/3-Structure.stories.tsx
Outdated
|
||
tabPicker.story = { | ||
name: 'Tab Picker', | ||
parameters: { note: 'used in picker pop-over, but also can be used whereever we need tabs' } |
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.
Spelling: whereever -> wherever
stories/3-Structure.stories.tsx
Outdated
} | ||
} | ||
|
||
const allColumnTypes: DataTypes[] = ['string', 'integer', 'number', 'boolean', 'null', 'any'] |
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.
No 'array' or 'object'? Could you add a comment mentioning why they're not here?
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.
oversight, just kept forgetting them. adding them in.
import * as React from 'react' | ||
import Icon from '../chrome/Icon' | ||
|
||
export type DataTypes = 'string' | 'integer' | 'number' | 'boolean' | 'null' | 'any' | 'array' | 'object' |
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.
This list of types appears in four places: here, app/components/overlay/TypePickerOverlay.tsx, app/components/structure/TypePicker.tsx, and stories/3-Structure.stories.tsx. Could we line-up them up everywhere so that they always follow the same order? It seems like app/components/structure/TypePicker.tsx has the order that matters most, which would mean changing it here.
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.
Okay cool. Now there is one place defining the different strings that count as DataTypes
, that is in the DataType
component referenced here.
For the actual array of types (defining the order and also adding descriptions), we reference the typesAndDescriptions
array defined in TypePicker
run
yarn storybook
to view componentadded components:
onCancel
if you click outside fo the div, or click the 'x'. Thenavigation
prop is optional. Anychildren
will overflow-y scroll.DataTypes
type. Wraps the type name, description, and icon.type
anddescription
, are defined and passed down. May want to refactor to keep description and type defined here, and just use the type to determine all content.Overlay
that lists the DataTypes and passes through thenavigation
component.typesAndDescriptions
are defined here.${column-name}-type
)This commit also created scss files based on the folder structure:
It also adds a story for each component under the
structure
tabcloses #384