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

Exclude images from being bundled with gem #1142

Merged
merged 4 commits into from
Jan 23, 2023
Merged

Exclude images from being bundled with gem #1142

merged 4 commits into from
Jan 23, 2023

Conversation

m-r-mccormick
Copy link
Contributor

PR #1124 was split into two PRs, this being one of them. This excludes images from being included in the gem file which propagates to downstream sites. search.svg is the only image file remaining which is used in downstream sites, although I couldn't find it referenced anywhere in the project, and excluding it doesn't appear to negatively impact downstream sites. In addition, search.svg doesn't appear to be the same search icon used in the search input of downstream sites: They are different orientations and shapes/sizes.

This excludes from downstream sites:

assets/images/large-image.jpg
assets/images/small-image.jpg
favicon.ico

Copy link
Member

@mattxwang mattxwang left a comment

Choose a reason for hiding this comment

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

Is there a reason that we're ignoring the favicon? My understanding is that we intentionally package it by default (since it beats not having a favicon) - see it being included in the .match statement.

@mattxwang mattxwang changed the title Exclude Images From Gem Exclude images from being bundled with gem Jan 23, 2023
@m-r-mccormick
Copy link
Contributor Author

I don't have a solid reason, and I think you are right that having it as a default is better than not having one. Just pushed an update.

Copy link
Member

@mattxwang mattxwang left a comment

Choose a reason for hiding this comment

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

Thanks for the quick turnaround. Testing locally, the new git ls-files command works as intended.

We'll only really get this tested when I actually push out the next release, but I think I'm happy to merge this in as-is. Thanks for your contribution!

@mattxwang mattxwang merged commit 3335b97 into just-the-docs:main Jan 23, 2023
@m-r-mccormick m-r-mccormick deleted the dev3 branch January 23, 2023 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants