You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/09/04 21:23:22 UTC

[GitHub] [incubator-superset] eschutho opened a new pull request #10799: use svg for checkbox component

eschutho opened a new pull request #10799:
URL: https://github.com/apache/incubator-superset/pull/10799


   ### SUMMARY
   Replaces the use of font-awesome for checkboxes with the svg components that already exist. There's also the potential to add the existing "half-checked" svg icon to the checkbox component as a prop as a future improvement.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   There are some minor visual differences with the new component: 
   
   Before:
   ![Screen Recording 2020-09-04 at 12 12 06 PM](https://user-images.githubusercontent.com/5186919/92285586-8242d580-eeb9-11ea-9157-87566e07ddc0.gif)
   
   After:
   ![Screen Recording 2020-09-04 at 12 15 21 PM](https://user-images.githubusercontent.com/5186919/92285558-71925f80-eeb9-11ea-8011-79861ac975e6.gif)
   
   Note: there is no focus state for this new component, and the color is more green than blue. 
   
   ### TEST PLAN
   A story and test were added, but it can be manually tested in the save dashboard modal as in the example.
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [x ] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   


----------------------------------------------------------------
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.

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

[GitHub] [incubator-superset] codecov-commenter edited a comment on pull request #10799: use svg for checkbox component

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10799:
URL: https://github.com/apache/incubator-superset/pull/10799#issuecomment-689783554


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10799?src=pr&el=h1) Report
   > Merging [#10799](https://codecov.io/gh/apache/incubator-superset/pull/10799?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/f6858256f496387146c4b535436bedaeceeeee51?el=desc) will **decrease** coverage by `0.86%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/10799/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/10799?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #10799      +/-   ##
   ==========================================
   - Coverage   61.22%   60.36%   -0.87%     
   ==========================================
     Files         802      373     -429     
     Lines       37814    23875   -13939     
     Branches     3555        0    -3555     
   ==========================================
   - Hits        23153    14412    -8741     
   + Misses      14475     9463    -5012     
   + Partials      186        0     -186     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #javascript | `?` | |
   | #python | `60.36% <ø> (-0.65%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10799?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset/db\_engines/hive.py](https://codecov.io/gh/apache/incubator-superset/pull/10799/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
   | [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/incubator-superset/pull/10799/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `53.90% <0.00%> (-30.08%)` | :arrow_down: |
   | [superset/db\_engine\_specs/mysql.py](https://codecov.io/gh/apache/incubator-superset/pull/10799/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL215c3FsLnB5) | `79.16% <0.00%> (-12.50%)` | :arrow_down: |
   | [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/incubator-superset/pull/10799/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `70.85% <0.00%> (-11.44%)` | :arrow_down: |
   | [superset/examples/world\_bank.py](https://codecov.io/gh/apache/incubator-superset/pull/10799/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXhhbXBsZXMvd29ybGRfYmFuay5weQ==) | `97.10% <0.00%> (-2.90%)` | :arrow_down: |
   | [superset/examples/birth\_names.py](https://codecov.io/gh/apache/incubator-superset/pull/10799/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXhhbXBsZXMvYmlydGhfbmFtZXMucHk=) | `97.36% <0.00%> (-2.64%)` | :arrow_down: |
   | [superset/views/database/mixins.py](https://codecov.io/gh/apache/incubator-superset/pull/10799/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `80.70% <0.00%> (-1.76%)` | :arrow_down: |
   | [superset/models/core.py](https://codecov.io/gh/apache/incubator-superset/pull/10799/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `86.66% <0.00%> (-1.67%)` | :arrow_down: |
   | [superset/views/core.py](https://codecov.io/gh/apache/incubator-superset/pull/10799/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `74.24% <0.00%> (-0.25%)` | :arrow_down: |
   | [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/incubator-superset/pull/10799/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `89.48% <0.00%> (-0.25%)` | :arrow_down: |
   | ... and [432 more](https://codecov.io/gh/apache/incubator-superset/pull/10799/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10799?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10799?src=pr&el=footer). Last update [f685825...b45d96a](https://codecov.io/gh/apache/incubator-superset/pull/10799?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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.

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

[GitHub] [incubator-superset] codecov-commenter commented on pull request #10799: use svg for checkbox component

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #10799:
URL: https://github.com/apache/incubator-superset/pull/10799#issuecomment-689783554


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10799?src=pr&el=h1) Report
   > Merging [#10799](https://codecov.io/gh/apache/incubator-superset/pull/10799?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/f6858256f496387146c4b535436bedaeceeeee51?el=desc) will **decrease** coverage by `1.25%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/10799/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/10799?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #10799      +/-   ##
   ==========================================
   - Coverage   61.22%   59.97%   -1.26%     
   ==========================================
     Files         802      373     -429     
     Lines       37814    23864   -13950     
     Branches     3555        0    -3555     
   ==========================================
   - Hits        23153    14313    -8840     
   + Misses      14475     9551    -4924     
   + Partials      186        0     -186     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #javascript | `?` | |
   | #python | `59.97% <ø> (-1.04%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10799?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset/db\_engines/hive.py](https://codecov.io/gh/apache/incubator-superset/pull/10799/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
   | [superset/databases/commands/create.py](https://codecov.io/gh/apache/incubator-superset/pull/10799/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL2NyZWF0ZS5weQ==) | `31.91% <0.00%> (-59.58%)` | :arrow_down: |
   | [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/incubator-superset/pull/10799/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `53.90% <0.00%> (-30.08%)` | :arrow_down: |
   | [superset/views/database/mixins.py](https://codecov.io/gh/apache/incubator-superset/pull/10799/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `59.64% <0.00%> (-22.81%)` | :arrow_down: |
   | [superset/databases/schemas.py](https://codecov.io/gh/apache/incubator-superset/pull/10799/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL3NjaGVtYXMucHk=) | `84.24% <0.00%> (-14.98%)` | :arrow_down: |
   | [superset/db\_engine\_specs/mysql.py](https://codecov.io/gh/apache/incubator-superset/pull/10799/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL215c3FsLnB5) | `79.16% <0.00%> (-12.50%)` | :arrow_down: |
   | [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/incubator-superset/pull/10799/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `70.85% <0.00%> (-11.44%)` | :arrow_down: |
   | [superset/databases/api.py](https://codecov.io/gh/apache/incubator-superset/pull/10799/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2FwaS5weQ==) | `83.54% <0.00%> (-8.87%)` | :arrow_down: |
   | [superset/databases/commands/update.py](https://codecov.io/gh/apache/incubator-superset/pull/10799/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `85.71% <0.00%> (-8.17%)` | :arrow_down: |
   | [superset/databases/dao.py](https://codecov.io/gh/apache/incubator-superset/pull/10799/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2Rhby5weQ==) | `92.85% <0.00%> (-7.15%)` | :arrow_down: |
   | ... and [445 more](https://codecov.io/gh/apache/incubator-superset/pull/10799/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10799?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10799?src=pr&el=footer). Last update [f685825...b45d96a](https://codecov.io/gh/apache/incubator-superset/pull/10799?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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.

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

[GitHub] [incubator-superset] rusackas commented on a change in pull request #10799: use svg for checkbox component

Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10799:
URL: https://github.com/apache/incubator-superset/pull/10799#discussion_r483884398



##########
File path: superset-frontend/src/components/Checkbox/Checkbox.test.tsx
##########
@@ -0,0 +1,72 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { ReactWrapper, shallow } from 'enzyme';

Review comment:
       Just FYI, this `shallow` will break if we use any Emotion theme vars in the component (which we probably will before long, e.g. to affect the color(s) of the SVG). the theming helpers one line below also include a "styledShallow" you can import to solve that.




----------------------------------------------------------------
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.

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

[GitHub] [incubator-superset] eschutho commented on a change in pull request #10799: use svg for checkbox component

Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #10799:
URL: https://github.com/apache/incubator-superset/pull/10799#discussion_r485280975



##########
File path: superset-frontend/src/components/Checkbox/Checkbox.stories.jsx
##########
@@ -0,0 +1,46 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React from 'react';
+import { useArgs } from '@storybook/client-api';
+import Checkbox from './';
+
+export default {
+  title: 'Checkbox',
+  component: Checkbox,
+};
+
+// eslint-disable-next-line no-unused-vars
+export const Component = _args => {
+  const [{ checked, style }, updateArgs] = useArgs();

Review comment:
       FWICT storybook seems to be trying to move people to args instead of knobs, since args is part of original package instead of an addon: https://github.com/storybookjs/storybook/blob/next/addons/controls/README.md#knobs-to-custom-args
   I put together a draft of the buttons as an arg/controls to see if we could migrate knobs to args, and it seems to work, but wdyt?
   
   Here's a view of the checkbox and button as args/controls (ignore the knob stories for now): 
   ![Screen Recording 2020-09-08 at 6 12 57 PM](https://user-images.githubusercontent.com/5186919/92543141-7d568c80-f1ff-11ea-8807-ae8c85f577a9.gif)
   
   




----------------------------------------------------------------
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.

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

[GitHub] [incubator-superset] mistercrunch commented on pull request #10799: use svg for checkbox component

Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #10799:
URL: https://github.com/apache/incubator-superset/pull/10799#issuecomment-687677074


   Welcome to the repo and community @eschutho !
   
   ![welcome](https://user-images.githubusercontent.com/487433/92315215-67469300-ef97-11ea-87cb-712e59fed66a.gif)
   


----------------------------------------------------------------
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.

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

[GitHub] [incubator-superset] rusackas commented on a change in pull request #10799: use svg for checkbox component

Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10799:
URL: https://github.com/apache/incubator-superset/pull/10799#discussion_r483884742



##########
File path: superset-frontend/src/components/Checkbox/Checkbox.stories.jsx
##########
@@ -0,0 +1,46 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React from 'react';
+import { useArgs } from '@storybook/client-api';
+import Checkbox from './';
+
+export default {
+  title: 'Checkbox',
+  component: Checkbox,
+};
+
+// eslint-disable-next-line no-unused-vars
+export const Component = _args => {
+  const [{ checked, style }, updateArgs] = useArgs();

Review comment:
       Pretty cool, I didn't know this even existed, actually. Nice that it lets the components do the toggling.
   
   I still like the idea of knobs, too, so you can control any given component from a common location. I wonder if there's a way that both can co-exist, e.g. this `updateArgs` toggles the `checked` prop _and_ toggles the knob's checkbox.




----------------------------------------------------------------
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.

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

[GitHub] [incubator-superset] eschutho commented on a change in pull request #10799: use svg for checkbox component

Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #10799:
URL: https://github.com/apache/incubator-superset/pull/10799#discussion_r485276785



##########
File path: superset-frontend/src/components/Checkbox/Checkbox.test.tsx
##########
@@ -0,0 +1,72 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { ReactWrapper, shallow } from 'enzyme';

Review comment:
       Looks like the styledShallow won't work without the HOC from Emotion because of the `dive` after the shallow render, but I think it could be a good plan to add emotion to this anyway.




----------------------------------------------------------------
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.

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

[GitHub] [incubator-superset] eschutho commented on a change in pull request #10799: use svg for checkbox component

Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #10799:
URL: https://github.com/apache/incubator-superset/pull/10799#discussion_r485279977



##########
File path: superset-frontend/src/components/Checkbox/Checkbox.stories.jsx
##########
@@ -0,0 +1,46 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React from 'react';
+import { useArgs } from '@storybook/client-api';
+import Checkbox from './';
+
+export default {
+  title: 'Checkbox',
+  component: Checkbox,
+};
+

Review comment:
       Sounds good. Now it makes sense why the button story has a gallery and interactive view. :)




----------------------------------------------------------------
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.

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

[GitHub] [incubator-superset] eschutho commented on pull request #10799: use svg for checkbox component

Posted by GitBox <gi...@apache.org>.
eschutho commented on pull request #10799:
URL: https://github.com/apache/incubator-superset/pull/10799#issuecomment-693013205


   @rusackas my bad for not checking up on this after rebasing! Thanks for calling it out. 


----------------------------------------------------------------
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.

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

[GitHub] [incubator-superset] rusackas commented on a change in pull request #10799: use svg for checkbox component

Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10799:
URL: https://github.com/apache/incubator-superset/pull/10799#discussion_r483884833



##########
File path: superset-frontend/src/components/Checkbox/Checkbox.stories.jsx
##########
@@ -0,0 +1,46 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React from 'react';
+import { useArgs } from '@storybook/client-api';
+import Checkbox from './';
+
+export default {
+  title: 'Checkbox',
+  component: Checkbox,
+};
+

Review comment:
       It might also be nice to add a Gallery story that just lays out all the variations to see, like they are in Figma




----------------------------------------------------------------
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.

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

[GitHub] [incubator-superset] rusackas merged pull request #10799: feat: use svg for checkbox component

Posted by GitBox <gi...@apache.org>.
rusackas merged pull request #10799:
URL: https://github.com/apache/incubator-superset/pull/10799


   


----------------------------------------------------------------
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.

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

[GitHub] [incubator-superset] eschutho edited a comment on pull request #10799: use svg for checkbox component

Posted by GitBox <gi...@apache.org>.
eschutho edited a comment on pull request #10799:
URL: https://github.com/apache/incubator-superset/pull/10799#issuecomment-689789884


   @rusackas I updated the button story to use args/controls...  it's about the same amount of code, but there are implicit args that you get for free. See what you think. 


----------------------------------------------------------------
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.

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

[GitHub] [incubator-superset] rusackas commented on a change in pull request #10799: use svg for checkbox component

Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10799:
URL: https://github.com/apache/incubator-superset/pull/10799#discussion_r485932477



##########
File path: superset-frontend/src/components/Checkbox/Checkbox.stories.jsx
##########
@@ -0,0 +1,46 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React from 'react';
+import { useArgs } from '@storybook/client-api';
+import Checkbox from './';
+
+export default {
+  title: 'Checkbox',
+  component: Checkbox,
+};
+
+// eslint-disable-next-line no-unused-vars
+export const Component = _args => {
+  const [{ checked, style }, updateArgs] = useArgs();

Review comment:
       Args/controls LGTM!




----------------------------------------------------------------
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.

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

[GitHub] [incubator-superset] rusackas commented on a change in pull request #10799: use svg for checkbox component

Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10799:
URL: https://github.com/apache/incubator-superset/pull/10799#discussion_r485134478



##########
File path: superset-frontend/src/components/Checkbox/index.tsx
##########
@@ -26,22 +30,16 @@ interface CheckboxProps {
 
 export default function Checkbox({ checked, onChange, style }: CheckboxProps) {
   return (
-    <span style={style}>
-      <i
-        role="button"
-        tabIndex={0}
-        className={`fa fa-check ${
-          checked ? 'text-primary' : 'text-transparent'
-        }`}
-        onClick={() => {
-          onChange(!checked);
-        }}
-        style={{
-          border: '1px solid #aaa',
-          borderRadius: '2px',
-          cursor: 'pointer',
-        }}
-      />
+    <span
+      style={{ verticalAlign: 'top', ...style }}

Review comment:
       We could probably use emotion to make this `<span...` a `<Styles...` tag, using Emotion, to address this alignment and the piecemeal ones added to the SVG files, like so:
   
   ```
   const Styles=styled.span`
     vertical-align: top;
     ...
     svg {
       vertical-align: top;
     }
   `
   ```




----------------------------------------------------------------
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.

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

[GitHub] [incubator-superset] rusackas commented on pull request #10799: use svg for checkbox component

Posted by GitBox <gi...@apache.org>.
rusackas commented on pull request #10799:
URL: https://github.com/apache/incubator-superset/pull/10799#issuecomment-692220023


   @eschutho I think this one's ready to rock, but just needs a little touchup to pass CI. Sorry it sat here so long! One thing I see causing CI to fail is that `styled` should now be imported from `@superset-ui/core` (due to the big consolidation effort there)


----------------------------------------------------------------
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.

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

[GitHub] [incubator-superset] eschutho commented on pull request #10799: use svg for checkbox component

Posted by GitBox <gi...@apache.org>.
eschutho commented on pull request #10799:
URL: https://github.com/apache/incubator-superset/pull/10799#issuecomment-689789884


   @rusackas I updated the button story to use args/controls...  it's about the same amount of code, but you there are implicit args that you get for free. See what you think. 


----------------------------------------------------------------
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.

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