Update design elements #1248
Update design elements #1248
Conversation
|
|
||
| return FieldListItem; | ||
| }; | ||
| export function FieldListItemFactoryFactory(FieldToken) { |
There was a problem hiding this comment.
FieldListItemFactoryFactory -> FieldListItemFactory?
There was a problem hiding this comment.
the previous component had already "Factory" in its name, but wasn't a kepler factory
| @@ -48,6 +49,17 @@ const StyledSliderHandle = styled.span` | |||
| background-color: ${props => props.theme.sliderHandleHoverColor}; | |||
| cursor: pointer; | |||
| } | |||
|
|
|||
| //// | |||
| } | ||
| }; | ||
|
|
||
| // NewFilterPanelFactory.deps = [ |
There was a problem hiding this comment.
this shouldn't be comment out
There was a problem hiding this comment.
sorry, my mistake. fixed
| @@ -124,13 +124,13 @@ export {default as CloudTile} from './modals/cloud-tile'; | |||
| export {default as FileUploadFactory, FileUpload} from './common/file-uploader/file-upload'; | |||
| export {default as DatasetLabel} from './common/dataset-label'; | |||
| export {default as ItemSelector} from './common/item-selector/item-selector'; | |||
| export {default as FieldSelector} from './common/field-selector'; | |||
There was a problem hiding this comment.
this will break the app that is importing FieldSelector
take a look at how TimeRangeSliderFactory and TimeRangeSlider are exported and do this same
There was a problem hiding this comment.
Fixed. nice catch, feel free to take a look
| export {default as Modal, ModalFooter, ModalTitle} from './common/modal'; | ||
| export {default as AppLogo} from './common/logo'; | ||
| export {default as Switch} from './common/switch'; | ||
| export {default as LoadingSpinner} from './common/loading-spinner'; | ||
| export {default as LoadingDialog} from './modals/loading-dialog'; | ||
| export {default as FieldToken} from './common/field-token'; |
There was a problem hiding this comment.
same, fixed
| @@ -181,6 +180,8 @@ function TooltipConfigFactory(DatasetTag) { | |||
| } | |||
|
|
|||
| return injectIntl(TooltipConfig); | |||
| } | |||
| // return TooltipConfig; | |||
src/components/common/field-token.js
Outdated
| text-align: center; | ||
| width: 40px; | ||
| `; | ||
| const ORANGE = '248, 194, 28'; |
There was a problem hiding this comment.
why do you move these constant out from default-settings and into field-token.js ? You don't need to move them into the factory if they are not depends on any deps of factory
There was a problem hiding this comment.
these contants are being using just on the field-token, for all the other colors we're using the theme. that's why it makes more sense to move them inside the token component
There was a problem hiding this comment.
no, please leave the constatns in the constants folder, because they might be used by other files in the future
src/styles/base.js
Outdated
| @@ -199,6 +204,7 @@ export const panelHeaderHeight = 48; | |||
| export const panelBoxShadow = '0 6px 12px 0 rgba(0,0,0,0.16)'; | |||
| export const panelBorderRadius = '2px'; | |||
| export const panelBackgroundLT = '#F8F8F9'; | |||
| export const styledLayerConfigGroupLabelLabelFontSize = '12px'; | |||
There was a problem hiding this comment.
remove styled in theme variable name. Also address variables added in you previous PR that has styled
There was a problem hiding this comment.
Good catch, just fixed!
| NewFilterPanelFactory.deps = [FilterPanelHeaderFactory, SourceDataSelectorFactory]; | ||
|
|
||
| function NewFilterPanelFactory(FilterPanelHeader, SourceDataSelector) { | ||
| const NewFilterPanelFactory = (FilterPanelHeader, SourceDataSelector, FieldSelector) => { |
There was a problem hiding this comment.
make NewFilterPanelFactory a actual function, instead of const ... = () => {} so you won't have errors like xxx is not declared before it is being used, and put the deps definition on top of the function declaration for easy read.
make it
NewFilterPanelFactory.deps = [...];
function NewFilterPanelFactory(...) {
...
}
| @@ -68,7 +67,7 @@ const CompareSwitchWrapper = styled.div` | |||
| margin-bottom: 8px; | |||
| `; | |||
|
|
|||
| function TooltipConfigFactory(DatasetTag) { | |||
| const TooltipConfigFactory = (DatasetTag, FieldSelector) => { | |||
There was a problem hiding this comment.
same here, make it a function instead of const ...
put the deps definition on top of the function declaration for easy read.
| } | ||
| return LayerColumnConfig; | ||
| }; | ||
| const ColumnSelectorFactory = FieldSelector => { |
There was a problem hiding this comment.
same here, make it a function instead of const ...
put the deps definition on top of the function declaration for easy read.
| @@ -103,9 +102,7 @@ const StyledTitle = styled(CenterFlexbox)` | |||
| } | |||
| `; | |||
|
|
|||
| TimeWidgetFactory.deps = [SpeedControlFactory, TimeRangeFilterFactory, FloatingTimeDisplayFactory]; | |||
|
|
|||
| function TimeWidgetFactory(SpeedControl, TimeRangeFilter, FloatingTimeDisplay) { | |||
There was a problem hiding this comment.
put the deps definition on top of the function declaration for easy read.
| fieldPairs: PropTypes.arrayOf(PropTypes.any), | ||
| columnLabels: PropTypes.object | ||
| }; | ||
| const LayerColumnConfigFactory = ColumnSelector => { |
There was a problem hiding this comment.
same here, make it a function instead of const ...
put the deps definition on top of the function declaration for easy read.
| TextLabelPanelFactory.deps = [RangeSliderFactory, LayerConfigGroupFactory]; | ||
|
|
||
| export default function TextLabelPanelFactory(RangeSlider, LayerConfigGroup) { | ||
| const TextLabelPanelFactory = (RangeSlider, LayerConfigGroup, FieldSelector) => { |
There was a problem hiding this comment.
same here, make it a function instead of const ...
put the deps definition on top of the function declaration for easy read.
|
|
||
| VisConfigByFieldSelectorFacotry.deps = [InfoHelperFacotry]; | ||
| export default function VisConfigByFieldSelectorFacotry(InfoHelper) { | ||
| const VisConfigByFieldSelectorFactory = (InfoHelper, FieldSelector) => { |
There was a problem hiding this comment.
same here, make it a function instead of const ...
put the deps definition on top of the function declaration for easy read.
src/styles/base.js
Outdated
| export const secondaryInputBgd = '#242730'; | ||
| export const secondaryInputBgdHover = '#3A414C'; | ||
| export const secondaryInputBgdActive = '#3A414C'; | ||
| export const secondaryInputColor = '#A0A7B4'; | ||
| export const secondaryInputBorderColor = '#242730'; | ||
| export const secondaryInputBorderActiveColor = '#D3D8E0'; | ||
| export const dropdownSelectHeight = '30px'; |
There was a problem hiding this comment.
make this 30 and in styled componenets use ${props => props.theme. dropdownSelectHeight}px
src/styles/base.js
Outdated
| @@ -47,6 +47,8 @@ export const titleColorLT = '#29323C'; | |||
| export const subtextColor = '#6A7485'; | |||
| export const subtextColorLT = '#A0A7B4'; | |||
| export const subtextColorActive = '#FFFFFF'; | |||
| export const subtextBorderColorActive = '#FFFFFF'; | |||
| export const subtextWidth = '30px'; | |||
There was a problem hiding this comment.
what is it? why is a text need width
There was a problem hiding this comment.
thos belongs to the width of the panel toggles
| padding-bottom: 6px; | ||
| width: 30px; | ||
| width: ${props => props.theme.subtextWidth}; |
There was a problem hiding this comment.
should be panelTabWidth?
| @@ -1078,14 +1104,21 @@ export const theme = { | |||
| sidePanelScrollBarWidth, | |||
| sidePanelScrollBarHeight, | |||
| sidePanelTitleFontsize, | |||
| sidePanelTitleLineHeight, | |||
There was a problem hiding this comment.
Good catch! I forgot to add it, done.
| @@ -46,13 +46,14 @@ const PanelTab = styled.div.attrs({ | |||
| align-items: flex-end; | |||
| border-bottom-style: solid; | |||
| border-bottom-width: 2px; | |||
| border-bottom-color: ${props => (props.active ? props.theme.subtextColorActive : 'transparent')}; | |||
| border-bottom-color: ${props => | |||
| props.active ? props.theme.subtextBorderColorActive : 'transparent'}; | |||
There was a problem hiding this comment.
this should be named panelToggleBorderColor
src/styles/base.js
Outdated
| @@ -258,10 +267,17 @@ export const sliderBarHeight = 4; | |||
| export const sliderHandleHeight = 12; | |||
| export const sliderHandleWidth = 12; | |||
| export const sliderHandleColor = '#D3D8E0'; | |||
| export const sliderHandleTextColor = 'black'; | |||
There was a problem hiding this comment.
why black it is coming out of nowhere? make it a same as sliderHandleColor since you are not showing anything
src/styles/base.js
Outdated
| export const sliderHandleHoverColor = '#FFFFFF'; | ||
| export const sliderHandleText = ''; |
There was a problem hiding this comment.
rename to sliderHandleAfterContent
src/styles/base.js
Outdated
| @@ -323,6 +339,9 @@ const progressBarTrackColor = '#E8E8E8'; | |||
| export const actionPanelWidth = 110; | |||
| export const actionPanelHeight = 32; | |||
|
|
|||
| // Styled Token | |||
| export const tokenRightMargin = 4; | |||
There was a problem hiding this comment.
rename to fieldTokenRightMargin
| @@ -24,6 +24,8 @@ import VisConfigSliderFactory from './side-panel/layer-panel/vis-config-slider'; | |||
| import LayerConfigGroupFactory from './side-panel/layer-panel/layer-config-group'; | |||
| import {ChannelByValueSelectorFactory} from './side-panel/layer-panel/layer-configurator'; | |||
| import {appInjector} from './container'; | |||
| import FieldSelectorFactory from './common/field-selector'; | |||
There was a problem hiding this comment.
nitpick, move the import above appInjector
Signed-off-by: Nicolas Rojo <nicolas.martin.rojo@gmail.com>
Signed-off-by: Shan He <heshan0131@gmail.com> Signed-off-by: Nicolas Rojo <nicolas.martin.rojo@gmail.com>
Signed-off-by: Shan He <heshan0131@gmail.com> Signed-off-by: Nicolas Rojo <nicolas.martin.rojo@gmail.com>
Signed-off-by: Shan He <heshan0131@gmail.com> Signed-off-by: Nicolas Rojo <nicolas.martin.rojo@gmail.com>
Signed-off-by: Nicolas Rojo <nicolas.martin.rojo@gmail.com>
Signed-off-by: Nicolas Rojo <nicolas.martin.rojo@gmail.com>
Signed-off-by: Nicolas Rojo <nicolas.martin.rojo@gmail.com>
Signed-off-by: Nicolas Rojo <nicolas.martin.rojo@gmail.com>
Signed-off-by: Shan He <heshan0131@gmail.com> Signed-off-by: Nicolas Rojo <nicolas.martin.rojo@gmail.com>
nicolasmartinrojo
force-pushed
the
feature/panelStyle
branch
from
8d43257 to
5776b81
Compare
…to feature/panelStyle
Signed-off-by: Nicolas Rojo <nicolas.martin.rojo@gmail.com>
Signed-off-by: Shan He <heshan0131@gmail.com> Signed-off-by: Nicolas Rojo <nicolas.martin.rojo@gmail.com>
Signed-off-by: Shan He <heshan0131@gmail.com> Signed-off-by: Nicolas Rojo <nicolas.martin.rojo@gmail.com>
Signed-off-by: Shan He <heshan0131@gmail.com> Signed-off-by: Nicolas Rojo <nicolas.martin.rojo@gmail.com>
Signed-off-by: Nicolas Rojo <nicolas.martin.rojo@gmail.com>
Signed-off-by: Nicolas Rojo <nicolas.martin.rojo@gmail.com>
Signed-off-by: Nicolas Rojo <nicolas.martin.rojo@gmail.com>
Signed-off-by: Nicolas Rojo <nicolas.martin.rojo@gmail.com>
Signed-off-by: Nicolas Rojo <nicolas.martin.rojo@gmail.com>
Signed-off-by: Shan He <heshan0131@gmail.com> Signed-off-by: Nicolas Rojo <nicolas.martin.rojo@gmail.com>
…to feature/panelStyle
Signed-off-by: Shan He <heshan0131@gmail.com>
Signed-off-by: Shan He <heshan0131@gmail.com>
…to feature/panelStyle
Signed-off-by: Shan He <heshan0131@gmail.com>
Signed-off-by: Shan He <heshan0131@gmail.com>
Signed-off-by: Shan He <heshan0131@gmail.com>
No description provided.