[GitHub] [superset] michael-s-molina commented on a diff in pull request #22474: chore: Migrate .less styles to Emotion

[GitBox <gi...@apache.org>]

michael-s-molina commented on code in PR #22474:
URL: https://github.com/apache/superset/pull/22474#discussion_r1064782780


##########
superset-frontend/src/SqlLab/SqlLabGlobalStyles.tsx:
##########
@@ -16,13 +16,25 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-@import '../../assets/stylesheets/less/variables.less';
 
-@import './builder.less';
-@import './dashboard.less';
-@import './dnd.less';
-@import './filter-scope-selector.less';
-@import './grid.less';
-@import './popover-menu.less';
-@import './resizable.less';
-@import './components/index.less';
+import React from 'react';
+import { Global } from '@emotion/react';
+import { css } from '@superset-ui/core';
+
+export const SqlLabGlobalStyles = () => (
+  <Global
+    styles={theme => css`
+      body {
+        min-height: max(
+          100vh,
+          ${theme.gridUnit * 125}px
+        ); // Set a min height so the gutter is always visible when resizing
+        overflow: hidden;
+      }
+
+      .cost-estimate {

Review Comment:
   It seems this is only used by the `EstimateQueryButton` component here:
   
   ```
   <TableView
     columns={columns}
     data={tableData}
     withPagination={false}
     emptyWrapperType={EmptyWrapperType.Small}
     className="cost-estimate"
   />
   ```
   
   Can you encapsulate `TableView` with `styled` and remove the class definition from the global config?



##########
superset-frontend/src/SqlLab/components/AceEditorWrapper/index.tsx:
##########
@@ -57,6 +59,28 @@ type AceEditorWrapperProps = {
   hotkeys: HotKey[];
 };
 
+const StyledAceEditor = styled(AceEditor)`
+  ${({ theme }) => css`
+    && {
+      //double class is better than !important
+      border: 1px solid ${theme.colors.grayscale.light2};
+      font-feature-settings: 'liga' off, 'calt' off;
+      // Fira Code causes problem with Ace under Firefox
+      font-family: 'Menlo', 'Consolas', 'Courier New', 'Ubuntu Mono',

Review Comment:
   Can we use fonts available in the theme?



##########
superset-frontend/src/SqlLab/components/AceEditorWrapper/index.tsx:
##########
@@ -57,6 +59,28 @@ type AceEditorWrapperProps = {
   hotkeys: HotKey[];
 };
 
+const StyledAceEditor = styled(AceEditor)`
+  ${({ theme }) => css`
+    && {
+      //double class is better than !important

Review Comment:
   ```suggestion
         // double class is better than !important
   ```



##########
superset-frontend/src/SqlLab/components/QueryStateLabel/QueryStateLabel.test.jsx:
##########
@@ -35,6 +33,6 @@ describe('SavedQuery', () => {
   });
   it('has an Overlay and a Popover', () => {
     const wrapper = shallow(<QueryStateLabel {...mockedProps} />);
-    expect(wrapper.find(Label)).toExist();
+    expect(wrapper.find('Label')).toExist();

Review Comment:
   ```suggestion
       expect(wrapper.find(Label)).toExist();
   ```



##########
superset-frontend/src/SqlLab/components/TabbedSqlEditors/TabbedSqlEditors.test.jsx:
##########
@@ -212,9 +212,9 @@ describe('TabbedSqlEditors', () => {
   });
   it('should disable new tab when offline', () => {
     wrapper = getWrapper();
-    expect(wrapper.find(EditableTabs).props().hideAdd).toBe(false);
+    expect(wrapper.find('EditableTabs').props().hideAdd).toBe(false);

Review Comment:
   ```suggestion
       expect(wrapper.find('#a11y-query-editor-tabs').props().hideAdd).toBe(false);
   ```



##########
superset-frontend/src/SqlLab/components/TabbedSqlEditors/TabbedSqlEditors.test.jsx:
##########
@@ -212,9 +212,9 @@ describe('TabbedSqlEditors', () => {
   });
   it('should disable new tab when offline', () => {
     wrapper = getWrapper();
-    expect(wrapper.find(EditableTabs).props().hideAdd).toBe(false);
+    expect(wrapper.find('EditableTabs').props().hideAdd).toBe(false);
     wrapper.setProps({ offline: true });
-    expect(wrapper.find(EditableTabs).props().hideAdd).toBe(true);
+    expect(wrapper.find('EditableTabs').props().hideAdd).toBe(true);

Review Comment:
   ```suggestion
       expect(wrapper.find('#a11y-query-editor-tabs').props().hideAdd).toBe(true);
   ```



##########
superset-frontend/src/SqlLab/components/QueryStateLabel/QueryStateLabel.test.jsx:
##########
@@ -18,8 +18,6 @@
  */
 import React from 'react';
 import { shallow } from 'enzyme';

Review Comment:
   You can replace `shallow` with `styledMount` and re-import `Label`:
   ```suggestion
   import { styledMount } from 'spec/helpers/theming';
   import Label from 'src/components/Label';
   ```



##########
superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx:
##########
@@ -228,7 +241,7 @@ const SqlEditorLeftBar = ({
   }, []);
 
   return (
-    <div data-test="sql-editor-left-bar" className="SqlEditorLeftBar">
+    <LeftBarStyles data-test="sql-editor-left-bar" className="SqlEditorLeftBar">

Review Comment:
   `className="SqlEditorLeftBar"` seems to only be used by Cypress tests but we could use the `data-test` attribute. Can you remove the `className`?



##########
superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.jsx:
##########
@@ -53,6 +53,14 @@ const defaultProps = {
   scheduleQueryWarning: null,
 };
 
+const StyledEditableTabs = styled(EditableTabs)`
+  height: 100%;
+  display: flex;
+  flex-direction: column;
+`;
+// Setting a displayName makes it easier to test with enzyme
+StyledEditableTabs.displayName = 'EditableTabs';

Review Comment:
   ```suggestion
   ```



##########
superset-frontend/src/SqlLab/components/QueryStateLabel/index.tsx:
##########
@@ -19,16 +19,20 @@
 import React from 'react';
 import Label from 'src/components/Label';
 import { STATE_TYPE_MAP } from 'src/SqlLab/constants';
-import { Query } from '@superset-ui/core';
+import { styled, Query } from '@superset-ui/core';
 
 interface QueryStateLabelProps {
   query: Query;
 }
 
+const StyledLabel = styled(Label)`
+  margin-right: ${({ theme }) => theme.gridUnit}px;
+`;
+// Setting a displayName makes it easier to test with enzyme
+StyledLabel.displayName = 'Label';

Review Comment:
   ```suggestion
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org