Remove Helper Classes from visualizations #4788
Conversation
| @@ -41,7 +41,7 @@ function DateTimeFormatSpecs() { | |||
| title={ | |||
| <React.Fragment> | |||
| Formatting Dates and Times | |||
| <i className="fa fa-external-link m-l-5" /> | |||
| <i className="fa fa-external-link" style={{ marginLeft: 5 }} /> | |||
There was a problem hiding this comment.
@kravets-levko I do try to avoid using inline styling, but replacing those isolated cases with only one css rule with it was really tempting
There was a problem hiding this comment.
Brilliant! That's what I always wanted to do but never had a chance 😄
| @@ -70,7 +71,12 @@ export default function withControlLabel(WrappedControl) { | |||
|
|
|||
| return ( | |||
| <ControlLabel layout={layout} label={label} labelProps={labelProps} disabled={disabled}> | |||
| <WrappedControl id={labelProps.htmlFor} disabled={disabled} {...props} /> | |||
| <WrappedControl | |||
| className={cx("visualization-editor-input", className)} | |||
There was a problem hiding this comment.
I forced all inputs to have width: 100% to remove the w-100 classes (all the InputNumbers and Select's had this class). LMK if you see any isolated case as I didn't find any on my checks.
There was a problem hiding this comment.
There should be no special cases, as far as I remember all editor inputs should have width: 100% and we use grid where needed
|
It doesn't matter much as you already handled it, but I wonder: assuming it's a very limited number of classes, why not just implement them in a dedicated stylesheet? |
|
@arikfr @gabrieldutra FYI - this PR is related to one of my PRs - #4588 |
I was already gonna have to check what were the ones being used and I didn't want to include those classes in the lib (helper classes seem something to be defined in an "App" scope, not in a lib and there's a remote possibility of conflicts), anyway the extra work to replace them seemed to be worth it. |
|
Hey there- I'm a bot, here to let you know that some code in this PR might not Please see that Pull Request's description for more details. |
gabrieldutra
force-pushed
the
visualizations-css-deps
branch
from
861c530
to
2009b66
Compare
| @@ -0,0 +1,5 @@ | |||
| .visualization-editor-tabs { | |||
| .ant-tabs-tab { | |||
| padding: 12px 16px; | |||
There was a problem hiding this comment.
Just a question: why was this needed? As far as I remember, we didn't change this anywhere.
There was a problem hiding this comment.
I have no direct idea (may be that we changed ant-tab style for all Redash or we may have a single conflicting variable), but without it, it looks like this on a blank project:
There was a problem hiding this comment.
Nevermind, my bad, just noticed I had antd v4 on that blank page 😬
| @blue: #2196f3; | ||
| @green: #4caf50; | ||
| @orange: #ff9800; | ||
| @redash-gray: rgba(102, 136, 153, 1); |
There was a problem hiding this comment.
Since all these variables will be independent from main Redash styles (and later we're going to redefine them for Redash) - I think we should avoid using too generic names and names from Redash styles. Instead, let's prefix all variables with some namespace to clearly see that they are related to visualizations and avoid names conflicts in the future. For example, this variable may be called @visualizations-gray: rgba(102, 136, 153, 1), and later, when using library in Redash - we'll define it as @visualizations-gray: @redash-gray. Same for others.
|
@kravets-levko I plan on merging this as soon as checks are approved in order to open a new PR with the separated package for visualizations, in case you have other comments you're welcome to make them and I'll address them in the upcoming PR 🙂. |
What type of PR is this? (check all applicable)
Description
This PR updates visualizations not to depend on helper classes such as
m-b-15ortext-nowrap.Related Tickets & Documents
--
Mobile & Desktop Screenshots/Recordings (if there are UI changes)
--