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

fix: Query History cosmetic issues #14885

Merged
merged 4 commits into from
Jun 1, 2021

Conversation

AAfghahi
Copy link
Member

@AAfghahi AAfghahi commented May 27, 2021

SUMMARY

Changed some cosmetic Issues on Query History in Sql Lab

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:
image

Now:
Screen Shot 2021-05-28 at 5 25 13 PM

TESTING INSTRUCTIONS

Go to SQL Lab, run a query, check out SQL Query History

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@AAfghahi AAfghahi force-pushed the ch16008_QueryHistoryCosmetic branch from 3308d7a to 8d049dc Compare May 27, 2021 22:14
@@ -57,12 +59,42 @@ const StaticPosition = css`
position: static;
`;

const verticalAlign = css`
Copy link
Member

Choose a reason for hiding this comment

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

can we move this to a styles.ts file?

@@ -98,6 +130,36 @@ const QueryTable = props => {
q.duration = fDuration(q.startDttm, q.endDttm);
}
const time = moment(q.startDttm).format().split('T');
const statusConfig = {
Copy link
Member

Choose a reason for hiding this comment

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

i'd move this into a function where we pass state and returns the updated statusConfig

const updateStatusConfig = (state) => {
 //...
}

@codecov
Copy link

codecov bot commented May 27, 2021

Codecov Report

Merging #14885 (45de2f0) into master (e466066) will decrease coverage by 0.03%.
The diff coverage is 64.01%.

❗ Current head 45de2f0 differs from pull request most recent head 14b16b3. Consider uploading reports for the commit 14b16b3 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14885      +/-   ##
==========================================
- Coverage   77.61%   77.58%   -0.04%     
==========================================
  Files         963      964       +1     
  Lines       49246    49329      +83     
  Branches     6197     6231      +34     
==========================================
+ Hits        38224    38270      +46     
- Misses      10821    10857      +36     
- Partials      201      202       +1     
Flag Coverage Δ
javascript 72.37% <59.80%> (-0.11%) ⬇️

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

Impacted Files Coverage Δ
superset-frontend/src/components/Form/FormItem.tsx 100.00% <ø> (ø)
...uperset-frontend/src/components/Menu/MenuRight.tsx 93.75% <ø> (ø)
...veFilters/FilterBar/FilterControls/FilterValue.tsx 67.44% <ø> (ø)
...mponents/nativeFilters/FiltersConfigModal/utils.ts 67.94% <ø> (-0.81%) ⬇️
...nd/src/dashboard/containers/DashboardComponent.jsx 84.84% <ø> (ø)
.../src/explore/components/ControlPanelsContainer.tsx 79.16% <0.00%> (-1.69%) ⬇️
...ntend/src/explore/components/ExploreChartPanel.jsx 16.66% <ø> (ø)
.../src/explore/components/controls/SelectControl.jsx 89.38% <ø> (ø)
...c/views/CRUD/data/database/DatabaseModal/styles.ts 97.36% <ø> (ø)
...rontend/src/visualizations/FilterBox/FilterBox.jsx 56.60% <0.00%> (ø)
... and 27 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 e466066...14b16b3. Read the comment docs.

@yousoph
Copy link
Member

yousoph commented May 27, 2021

On the headers, can you update "Sql" to "SQL"? Thank you :)

@AAfghahi AAfghahi force-pushed the ch16008_QueryHistoryCosmetic branch 3 times, most recently from 75ba4ad to a196579 Compare May 28, 2021 16:28
@AAfghahi AAfghahi force-pushed the ch16008_QueryHistoryCosmetic branch from a196579 to 8596e30 Compare May 28, 2021 16:29
@AAfghahi AAfghahi changed the title fix: Ch16008 query history cosmetic fix: Query History cosmetic issues May 28, 2021
Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

LGTM, just a comment on removing all the if logic.

label: '',
status: '',
};
if (q.state === 'success') {
Copy link
Member

Choose a reason for hiding this comment

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

To simplify some of the code here and where you set the color you could move all of this to a map:

statusAttributes = {
  success: {
    color: theme.colors.success.base,
    config: {
      name: 'check',
      label: t('Success'),
      status: 'success',
    },
  },
  ...
}

Then you can get the color with:

color: ${({ status, theme }) => statusAttributes[status]?.color || theme.colors.grayscale.base;

And here:

statusConfig = statusAttributes[q.state]?.config || defaultStatusConfig;

Copy link
Member Author

Choose a reason for hiding this comment

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

ohhhhhh I really like this.

@hughhhh hughhhh merged commit eced510 into apache:master Jun 1, 2021
@michellethomas
Copy link
Contributor

@AAfghahi I believe this PR created a few bugs in sqllab. It looks like the StatusAttributes do not all have the same structure, scheduled and pending do not have a config section or a color. Also there is a fetching status that has no entry in StatusAttributes. This causes an issue with statusAttributes[q.state].config here and down below because at times there is no config in statusAttributes or q.state doesn't exist in statusAttributes. Can you either make the statusAttributes data more complete or handle the undefined data in a safe way and create tests for these cases? cc: @betodealmeida @hughhhh

@AAfghahi
Copy link
Member Author

AAfghahi commented Jun 4, 2021

Hi @michellethomas I'll fix that now, will ping you when I have the PR setup

@michellethomas
Copy link
Contributor

Thanks for the quick response! If it helps with testing, it looks like the fetching status shows up when you click "View results" from the query history tab.

@AAfghahi
Copy link
Member Author

AAfghahi commented Jun 4, 2021

that's odd, the output for me is very different.
Screen Shot 2021-06-04 at 3 50 30 PM

Is what I have, not view results. Is there a featureFlag that I am missing?

Also as an addendum, we don't have a color on file for these, so I was thinking of making them the basic blue?

@michellethomas
Copy link
Contributor

When you run a successful query that returns data you don't see "View results"? I'll look around to see if we have something specific turned on for this.

image

@AAfghahi
Copy link
Member Author

AAfghahi commented Jun 4, 2021

nope! That's probably why I wasn't able to test it :)

@AAfghahi
Copy link
Member Author

AAfghahi commented Jun 4, 2021

@michellethomas #14995

I think this should do it? I checked in with our designer for the additional colors.

cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
* easiest fix

* Query History Cosmetic Issues

* added revisions

* beto revisions
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
* easiest fix

* Query History Cosmetic Issues

* added revisions

* beto revisions
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
* easiest fix

* Query History Cosmetic Issues

* added revisions

* beto revisions
@mistercrunch mistercrunch added the 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels label Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 1.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants