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.

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

chore: Replace Well componenet with Card component #12200

Closed

Conversation

nikolagigic
Copy link
Contributor

@nikolagigic nikolagigic commented Dec 24, 2020

SUMMARY

Replace react-bootstrap Well component with customised AntD Card component
#10254

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

  • Edit Dataset
    Screenshot 2020-12-24 at 14 38 36
  • SQLab. Query History
    Screenshot 2020-12-24 at 14 39 08
  • SQLab View Keys and Indexes
    Screenshot 2020-12-24 at 14 39 19

After:

  • Edit Dataset
    Screenshot 2020-12-24 at 14 33 58
  • SQLab. Query History
    Screenshot 2020-12-24 at 14 33 22
  • SQLab View Keys and Indexes
    Screenshot 2020-12-24 at 14 33 41

TEST PLAN

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@nikolagigic nikolagigic changed the title [chore]: Replace Well componenet with Card component chore: Replace Well componenet with Card component Dec 24, 2020
@codecov-io
Copy link

codecov-io commented Dec 24, 2020

Codecov Report

Merging #12200 (9e3a603) into master (a3bbbf8) will decrease coverage by 2.99%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12200      +/-   ##
==========================================
- Coverage   67.11%   64.12%   -3.00%     
==========================================
  Files         996      485     -511     
  Lines       49176    29867   -19309     
  Branches     4993        0    -4993     
==========================================
- Hits        33006    19151   -13855     
+ Misses      16047    10716    -5331     
+ Partials      123        0     -123     
Flag Coverage Δ
cypress ?
javascript ?
python 64.12% <ø> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/sql_validators/postgres.py 50.00% <0.00%> (-50.00%) ⬇️
superset/commands/importers/v1/examples.py 40.90% <0.00%> (-4.55%) ⬇️
superset/datasets/commands/importers/v1/utils.py 55.55% <0.00%> (-1.83%) ⬇️
superset/cli.py 33.58% <0.00%> (-0.92%) ⬇️
superset/reports/dao.py 78.50% <0.00%> (-0.75%) ⬇️
superset/views/base_api.py 97.68% <0.00%> (-0.47%) ⬇️
superset/databases/api.py 89.15% <0.00%> (ø)
superset/tasks/scheduler.py 0.00% <0.00%> (ø)
superset/async_events/api.py 100.00% <0.00%> (ø)
superset/reports/commands/log_prune.py 0.00% <0.00%> (ø)
... and 509 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3bbbf8...9e3a603. Read the comment docs.

Copy link
Member

@geido geido left a comment

Choose a reason for hiding this comment

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

Looks good but I think it is deviating from the approach that we recently took to enhance the Antdesign components using Emotion. Please have a look at my comments

superset-frontend/src/common/components/Card.tsx Outdated Show resolved Hide resolved
superset-frontend/src/common/components/Card.tsx Outdated Show resolved Hide resolved
@junlincc
Copy link
Member

junlincc commented Jan 1, 2021

Thanks for the PR @nikolagigic!
@kgabryje Hey Kamil, please meet Nikola from Flexiana. i wonder if you mind reviewing each other's PRs 😀

@junlincc junlincc requested a review from etr2460 January 1, 2021 22:29
@michael-s-molina
Copy link
Member

@nikolagigic Thanks for the PR. Can you please reference #10254 in the PR description?

@nikolagigic nikolagigic closed this Jan 4, 2021
@nikolagigic nikolagigic reopened this Jan 4, 2021
@nikolagigic nikolagigic closed this Jan 4, 2021
@nikolagigic nikolagigic reopened this Jan 4, 2021
@kgabryje
Copy link
Member

kgabryje commented Jan 4, 2021

Thanks for the PR @nikolagigic!
@kgabryje Hey Kamil, please meet Nikola from Flexiana. i wonder if you mind reviewing each other's PRs 😀

Not a problem 🙂 Hello @nikolagigic!

@geido
Copy link
Member

geido commented Jan 5, 2021

@nikolagigic Can we export the Card from src/common/components/index.tsx and maybe add a bit of storybook as well?

@rusackas rusackas added the hold! On hold label Jan 6, 2021
@rusackas rusackas removed the hold! On hold label Feb 2, 2021
@rusackas rusackas closed this Feb 18, 2021
@rusackas rusackas reopened this Feb 18, 2021
Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

@nikolagigic Sorry this one has been sitting here so long, but I think we'll be OK to merge it now. It looks like all comments were addressed, so I've marked them as resolved, and I'm approving.

I also saw the E2E tests were failing for some reason. I've closed/reopened to kick CI, so fingers crossed that it works 🤞 Please give this a rebase, and assess any failing Cypress tests if needed, and it should be good to go.

@junlincc Probably ought to give it a quick pass for Product/QA signoff before we merge, or delegate that job to anyone else she deems fit.

rusackas pushed a commit that referenced this pull request Apr 9, 2021
* replace Well componenet with Card component

* Fix lint errors

* Fix lint

* Addressing comments

* Addressing comments

* Fix lint errors

* Exporting Card from index.tsx and adding storybook variants

* Fix some refactoring

* Fix errors

* solving conflicts and lint errors

* Update superset-frontend/src/common/components/Card.tsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

* Update superset-frontend/src/common/components/Card.tsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

* Update superset-frontend/src/common/components/common.stories.tsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

* Update superset-frontend/src/common/components/common.stories.tsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

* Update superset-frontend/src/common/components/common.stories.tsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

* Update superset-frontend/src/common/components/common.stories.tsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

* Update superset-frontend/src/common/components/common.stories.tsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

Co-authored-by: Nikola Gigic <nik.gigic@gmail.com>
Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
allanco91 pushed a commit to allanco91/superset that referenced this pull request May 21, 2021
…ache#14033)

* replace Well componenet with Card component

* Fix lint errors

* Fix lint

* Addressing comments

* Addressing comments

* Fix lint errors

* Exporting Card from index.tsx and adding storybook variants

* Fix some refactoring

* Fix errors

* solving conflicts and lint errors

* Update superset-frontend/src/common/components/Card.tsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

* Update superset-frontend/src/common/components/Card.tsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

* Update superset-frontend/src/common/components/common.stories.tsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

* Update superset-frontend/src/common/components/common.stories.tsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

* Update superset-frontend/src/common/components/common.stories.tsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

* Update superset-frontend/src/common/components/common.stories.tsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

* Update superset-frontend/src/common/components/common.stories.tsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

Co-authored-by: Nikola Gigic <nik.gigic@gmail.com>
Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
@stale
Copy link

stale bot commented Jun 11, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

@stale stale bot added the inactive Inactive for >= 30 days label Jun 11, 2021
@stale stale bot closed this Jun 26, 2021
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
…ache#14033)

* replace Well componenet with Card component

* Fix lint errors

* Fix lint

* Addressing comments

* Addressing comments

* Fix lint errors

* Exporting Card from index.tsx and adding storybook variants

* Fix some refactoring

* Fix errors

* solving conflicts and lint errors

* Update superset-frontend/src/common/components/Card.tsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

* Update superset-frontend/src/common/components/Card.tsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

* Update superset-frontend/src/common/components/common.stories.tsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

* Update superset-frontend/src/common/components/common.stories.tsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

* Update superset-frontend/src/common/components/common.stories.tsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

* Update superset-frontend/src/common/components/common.stories.tsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

* Update superset-frontend/src/common/components/common.stories.tsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

Co-authored-by: Nikola Gigic <nik.gigic@gmail.com>
Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inactive Inactive for >= 30 days size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants