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

Layout components #145

Merged
merged 1 commit into from
Jun 21, 2021
Merged

Layout components #145

merged 1 commit into from
Jun 21, 2021

Conversation

HansKallekleiv
Copy link
Collaborator

Relates to #144

Adds a set of python component wrapping existing components from dash_html_components and dash_core_components.,
with e.g. additional styling and inclusion of labels.

anders-kiaer
anders-kiaer requested changes Jun 16, 2021
View reviewed changes
Copy link
Collaborator

@anders-kiaer anders-kiaer left a comment

Choose a reason for hiding this comment

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

🌟 🎨

.pylintrc Outdated
duplicate-code,
too-few-public-methods,
invalid-name,
too-many-arguments
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing new line. See this is the case for other files also.

react/package.json Outdated
@@ -22,7 +22,7 @@
"tidy-up": "rimraf ./R && rimraf ./dist && rimraf ./inst && rimraf ./man && rimraf ./.Rbuildignore",
"build:js": "webpack --mode production --entry ./dist/index.js",
"build:js-dev": "webpack --mode development --entry ./dist/index.js",
"build:py_and_r": "npm run transpile:dash && cp -a ../webviz_core_components/. ./webviz_core_components/ && cp ./package.json ./webviz_core_components/package.json && dash-generate-components ./dist/components webviz_core_components -p package.json --r-prefix '' --ignore '(^index.js|.stories.js)$' && cp -a ./webviz_core_components/. ../webviz_core_components/ && rimraf ./webviz_core_components",
"build:py_and_r": "npm run transpile:dash && cp -aR ../webviz_core_components/. ./webviz_core_components/ && cp ./package.json ./webviz_core_components/package.json && dash-generate-components ./dist/components webviz_core_components -p package.json --r-prefix '' --ignore '(^index.js|.stories.js)$' && cp -a ./webviz_core_components/. ../webviz_core_components/ && rimraf ./webviz_core_components",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just for understanding, what does adding -a solve?

Copy link
Collaborator

@rubenthoms rubenthoms Jun 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Since I added the a 😉
-a is a shortcut for -dR --preserve=all.

  • d: shortcut for --no-dereference --preserve=links (never follow symbolic links in SOURCE, preserve links)
  • R: copy directories recursively
  • --preserve=all: preserves all attributes (mode, ownership, timestamps..)

Hence, I am a bit surprised that you needed to add the R since it should be included in the -a flag 😕

Ref: https://man7.org/linux/man-pages/man1/cp.1.html

However, I was wondering for a while now if we should rather become platform independent and remove the cp entirely. There is a CLI version of fs-extra which provides all functionality we need. We could also replace rimraf then. See: https://www.npmjs.com/package/@atao60/fse-cli

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I you are right that R is not needed. Will remove!

react/src/lib/components/Layout/Slider/slider.css Outdated Show resolved Hide resolved
webviz_core_components/wrapped_components/slider.py
),
)
)
self.children = html.Div(style={"fontSize": "15px"}, children=children)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the utility here something that is now more naturally placed in this file?
https://github.com/equinor/webviz-config/blob/master/webviz_config/utils/_dash_component_utils.py

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good idea!

webviz_core_components/wrapped_components/tabs.py
*args: Any,
**kwargs: Any,
) -> None:
super().__init__(*args, **kwargs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does it do compared with dcc.Tabs? To be implemented?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mainly added for ease of use. It is used together with the Tab component which is wrapped here with some styling.

@anders-kiaer anders-kiaer added the next release 🚢 To be included in next release label Jun 17, 2021
@HansKallekleiv HansKallekleiv force-pushed the layout-components branch 3 times, most recently from ae88691 to 600a71f Compare June 21, 2021 11:34
anders-kiaer
anders-kiaer approved these changes Jun 21, 2021
View reviewed changes
react/src/lib/components/Layout/Slider/slider.css
Comment on lines +1 to +26
.webviz-slider .rc-slider-track {
background-color: var(--menuLinkColor) !important;
}

.webviz-slider .rc-slider-dot-active {
border-color: var(--menuLinkHoverColor) !important;
border-color: var(--menuLinkHoverColor) !important;
}

.webviz-slider .rc-slider-handle {
background-color: var(--menuLinkHoverColor) !important;
border-color: var(--menuLinkHoverColor) !important;
}

.webviz-slider .rc-slider-handle-active:active {
border-color: var(--menuLinkHoverColor) !important;
box-shadow: 0 0 5px var(--menuLinkHoverColor) !important;
}

.webviz-slider .rc-slider-dot {
margin-left:-10px !important;
background-color: var(--menuBackground) !important;
border-color: var(--menuLinkHoverColor) !important;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a comment here explaining why !important is necessary?

react/src/lib/components/Layout/Tab/tab.css
Comment on lines +1 to +16
.webviz-tab {
border-bottom: 1px solid #d6d6d6 !important;
padding: 6px !important;
font-weight: bold !important;
}

.webviz-tab-selected {
border-top: 1px solid #d6d6d6 !important;
border-bottom: 1px solid #d6d6d6 !important;
background-color: #007079 !important;
color: white !important;
padding: 6px !important;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

And here as well 🙂

react/src/lib/index.ts Outdated
Comment on lines 14 to 16
import "./components/Layout"
export { WebvizPluginPlaceholder, ColorScales, Select, SmartNodeSelector };
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
import "./components/Layout"
export { WebvizPluginPlaceholder, ColorScales, Select, SmartNodeSelector };
import "./components/Layout"
export { WebvizPluginPlaceholder, ColorScales, Select, SmartNodeSelector };

@HansKallekleiv HansKallekleiv merged commit c82b157 into equinor:master Jun 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rubenthoms rubenthoms rubenthoms left review comments

@anders-kiaer anders-kiaer anders-kiaer approved these changes

Assignees

@HansKallekleiv HansKallekleiv

Labels
next release 🚢 To be included in next release
Projects
Webviz
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@HansKallekleiv @anders-kiaer @rubenthoms