superset icon indicating copy to clipboard operation
superset copied to clipboard

feat(config): add LOADERS variable in config files to enable custom loader gif

Open ERGO1995 opened this issue 1 year ago • 10 comments

SUMMARY

Added a LOADERS variable in the configuration files config.py and superset_config.py:

  • config.py default value → LOADERS = [{"src": "/static/assets/images/loading.gif"}]
  • superset_config.py → LOADERS = [{"src": "/static/assets/images/YOUR_GIF.gif"}] (for exemple)

Managed the loader in Superset's base HTML skeleton:

  • superset/superset/templates/superset/basic.html

Retrieved the custom loader in Superset API endpoints:

  • superset/superset/views/core.py → Added an endpoint to retrieve the loader from the configuration file.

Managed the loader in Superset's React.js elements:

  • superset/superset-frontend/src/components/Loading/index.tsx → Fetches the custom loader via the new endpoint.

I'm very new to Superset and the open-source world, and this is my first attempt. I'm open to any advice on how to make this cleaner if needed :)

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Don't setup the LOADERS in superset_config.py file to see the default loader. Then add tyour custom gif to see the difference.

ADDITIONAL INFORMATION

  • [ ] Has associated issue:
  • [ ] Required feature flags:
  • [ ] 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
  • [x] Introduces new feature or API
  • [ ] Removes existing feature or API

ERGO1995 avatar Dec 19 '24 10:12 ERGO1995

Codecov Report

Attention: Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.

Project coverage is 83.75%. Comparing base (76d897e) to head (9682fc5). Report is 1259 commits behind head on master.

Files with missing lines Patch % Lines
superset/views/core.py 71.42% 2 Missing :warning:
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #31565       +/-   ##
===========================================
+ Coverage   60.48%   83.75%   +23.26%     
===========================================
  Files        1931      538     -1393     
  Lines       76236    39135    -37101     
  Branches     8568        0     -8568     
===========================================
- Hits        46114    32777    -13337     
+ Misses      28017     6358    -21659     
+ Partials     2105        0     -2105     
Flag Coverage Δ
hive 48.76% <77.77%> (-0.41%) :arrow_down:
javascript ?
mysql 76.51% <77.77%> (?)
postgres 76.57% <77.77%> (?)
presto 53.27% <77.77%> (-0.54%) :arrow_down:
python 83.75% <77.77%> (+20.26%) :arrow_up:
sqlite 76.03% <77.77%> (?)
unit 60.88% <77.77%> (+3.25%) :arrow_up:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Dec 19 '24 16:12 codecov[bot]

@rusackas thank you for your feedback. Yes, I agree that it would be cleaner. I'll take care of it.

As for the array, I followed the same approach as for the FAVICONS. I probably didn’t quite understand why they were in array form as well?

Otherwise, for this new element, I saw a GitHub issue mentioning loader customization. This could serve as a basis in case we want to add keys like the positioning of the loader or other ideas.

ERGO1995 avatar Dec 30 '24 08:12 ERGO1995

@rusackas I noticed that the loader is not handled properly in an embedding context. The image does not load. Do you have any idea what might be causing this issue? I came across a topic that could potentially relate to my problem, but the lines referenced in config.py no longer seem relevant (https://github.com/apache/superset/issues/28112#issuecomment-2062595472) @mistercrunch.

ERGO1995 avatar Dec 31 '24 09:12 ERGO1995

It feels like all this should be included in the theme object eventually.

mistercrunch avatar Jan 05 '25 01:01 mistercrunch

Heya... this hasn't been touched in a while, and has a bunch of CI issues, including pre-commit failures.

Superset uses Git pre-commit hooks courtesy of pre-commit. To install run the following:

pip3 install -r requirements/development.txt
pre-commit install
A series of checks will now run when you make a git commit.

Alternatively it is possible to run pre-commit by running pre-commit manually:

pre-commit run --all-files

I'm still not sure how to solve the embedded context issue, have you had any luck there? I suspect there needs to be a CSP adjustment made somewhere, but maybe you can see something in your browser logs?

rusackas avatar Mar 13 '25 18:03 rusackas

Heya... this hasn't been touched in a while, and has a bunch of CI issues, including pre-commit failures.

Superset uses Git pre-commit hooks courtesy of pre-commit. To install run the following:

pip3 install -r requirements/development.txt
pre-commit install
A series of checks will now run when you make a git commit.

Alternatively it is possible to run pre-commit by running pre-commit manually:

pre-commit run --all-files

I'm still not sure how to solve the embedded context issue, have you had any luck there? I suspect there needs to be a CSP adjustment made somewhere, but maybe you can see something in your browser logs?

Hey @rusackas, i don't understand cause i already use pre commit and i have this when i commit something: Capture d’écran 2025-03-14 à 15 12 36

ERGO1995 avatar Mar 14 '25 14:03 ERGO1995

Need a rebase on this one... but also I'm working on introducing a native SVG loader that's themable (as part of the newer Theming effort). I'll try to make sure we can fall back to a specified GIF (or whatever) if present.

rusackas avatar Jun 11 '25 17:06 rusackas

Wanted to note that the theming branch now includes tokens for the logo img, positioning and scaling. I believe it's probably the right place to also reference a custom spinner. This is likely to conflict with the theming branch and break the inteferface. Would recommend targeting the theming branch of waiting for it to merge to open something that's cohesive with the work taking place there.

mistercrunch avatar Jun 12 '25 17:06 mistercrunch

Here's a ref of how related things will be done in the future. I'd say we put this one on hold or try to target that branch. Note that there's a lot going on there and should merge into master soonish (few weeks at most (?))

https://github.com/apache/superset/blob/template_less/superset-frontend/packages/superset-ui-core/src/theme/Theme.tsx#L66-L70

mistercrunch avatar Jun 12 '25 17:06 mistercrunch

Was talking with @ERGO1995 on Slack... we'll wait a bit for this one. I'll just mark it as Draft for now.

rusackas avatar Jun 13 '25 15:06 rusackas

Closing this stale draft PR. It has been inactive for about 6 months and would need a rebase on master given the 12 months since it was opened.

I believe this functionality has largely been addressed by the Theming effort, but if there's more to do here, we welcome the discussion. Feel free to open an issue or fresh PR if you'd like to continue this work.

@ERGO1995 thanks for your contribution!

rusackas avatar Dec 11 '25 22:12 rusackas