-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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: Moves the images folder to the assets folder #14429
chore: Moves the images folder to the assets folder #14429
Conversation
Codecov Report
@@ Coverage Diff @@
## master #14429 +/- ##
==========================================
+ Coverage 76.67% 76.98% +0.31%
==========================================
Files 1005 1007 +2
Lines 54138 54174 +36
Branches 7442 7463 +21
==========================================
+ Hits 41510 41708 +198
+ Misses 12384 12226 -158
+ Partials 244 240 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
/testenv up |
@rusackas Ephemeral environment spinning up at http://52.12.116.219:8080. Credentials are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @michael-s-molina we need to make sure that the path is changed for the dynamic import of the Icons component as well. I don't see that path change included here
@geido This one? |
@michael-s-molina Is this in this PR? I have looked 3 times. I can't find it. |
@geido https://github.com/apache/superset/pull/14429/files#diff-4ee0a8eefdf7b42a39b21a7183b6559a4909da71533312d9eba61c569234862f |
That does not bring me to that change. I am from the Github app, maybe it is messed up due to the big amount of changes. But if you confirm it is there, then we are good @michael-s-molina |
@@ -94,14 +94,14 @@ | |||
<section> | |||
<h1>Page not found</h1> | |||
<p> | |||
Sorry, we cannot find t he page you are looking for. You may have | |||
Sorry, we cannot find the page you are looking for. You may have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch :)
/testenv up |
@rusackas Ephemeral environment spinning up at http://52.13.96.30:8080. Credentials are |
5eacfb9
to
16f363f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like a lot of the images in the directory are only used for documentation or on the docs website. can you verify before merging that they don't get deployed to the docker image, since they aren't needed to actually run the server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks!
16f363f
to
a34b8c5
Compare
I can confirm that I was not able to find the doc/website images in the docker image. So should be good to go 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Ephemeral environment shutdown and build artifacts deleted. |
* chore: Moves the images folder to the assets folder * Rebases
* chore: Moves the images folder to the assets folder * Rebases
SUMMARY
Moves the
images
folder to theassets
folder.This work is part of SIP-61
@rusackas @pkdotson please include anyone that needs to be aware of this. I tested the Storybook, development, and production versions and it seems ok. Please let me know if I forgot any dependency.
PS: Don't panic about the 317 files changed 😆, most are just file renames.
TEST PLAN
ADDITIONAL INFORMATION