Host image assets within package #722

emplums · 2019-03-12T21:07:17Z

Is your feature request related to a problem? Please describe.

Users of Primer/CSS outside of github/github cannot use form validation styles because the assets live in github/github

Describe the solution you'd like

Not sure what the solution here would be - either we add an /images folder to primer/css or host the images somewhere and provide full urls to them

Additional context
@gladwearefriends brought this up as the missing assets were causing her builds to fail on a project outside of github/github

The text was updated successfully, but these errors were encountered:

shawnbot · 2019-03-12T21:13:39Z

So we've got a couple of options here:

Remove the references to the github.com image URLs and prompt users to provide their own values. (Maybe via @warn in the stylesheets if the variables aren't set?)
Point to the fully-qualified URLs. (This might not be feasible because I think we have CORS restrictions in place?)
Leave the values as-is but move them to variables so that they can be overridden. This is probably the only one that wouldn't compel us to do a major version bump.

Ref: #442

zeke · 2019-03-12T22:54:48Z

Thanks for looking into this! I don't have a strong opinion about the best way to address it, but less configuration (or sensible defaults) would be ideal.

For the record, our project is not actually using any of the styles that have the missing images. We only discovered the problem by running an exhaustive link checker on the site.

It looks like they're all spinners or ajax-related images:

/images/spinners/octocat-spinner-16px.gif
/images/modules/ajax/success.png
/images/modules/ajax/error.png
/images/spinners/octocat-spinner-32.gif
/images/modules/ajax/success@2x.png
/images/modules/ajax/error@2x.png
/images/spinners/octocat-spinner-32-EAF2F5.gif

shawnbot · 2019-03-13T19:58:39Z

It looks like we don't serve any CORS headers that would prevent these from working anywhere if we qualified them with https://github.com:

% curl -I https://github.com/images/modules/ajax/error.png

HTTP/1.1 200 OK
Date: Wed, 13 Mar 2019 19:47:14 GMT
Content-Type: image/png
Content-Length: 218
Server: GitHub.com
Last-Modified: Wed, 13 Mar 2019 19:36:01 GMT
ETag: "5c895ba1-da"
Expires: Sat, 10 Mar 2029 19:47:14 GMT
Cache-Control: max-age=315360000
Accept-Ranges: bytes
X-Frame-Options: DENY
Vary: Accept-Encoding
X-GitHub-Request-Id: E719:04A9:B94F27:11BF21C:5C895E41

So I wonder if the best solution is to have a map of image URLs that can be selectively overridden? For instance:

$image-url-base: "https://github.com" !default;
$image-urls: (
  spinner-1x: "#{$image-url-base}/images/spinners/octocat-spinner-16px.gif",
  spinner-2x: "#{$image-url-base}/images/spinners/octocat-spinner-32.gif",
  // etc.
) !default;

and you could override them in your stylesheet with:

// override the url base, but preserve the paths
$image-url-base: "https://some-other-domain.com";
@import "@primer/css/support/index.scss";

// _or_ merge new values into the global $image-urls map after importing
$image-urls: map-merge($image-urls, (
  spinner-1x: "/path/to/spinner@1x.gif"
)) !global;

// then this picks up the variables we've set, because !default
@import "@primer/css/index.scss";

ocarreterom · 2019-03-13T20:43:08Z

Transform those images to base64 could be an option? 🤔

shawnbot · 2019-03-13T20:44:27Z

@ocarreterom It's an option, but that would significantly increase the size of our CSS bundles. We already do it for the ▼ in dropdown menus, but the base64 version of that image is very small.

skullface · 2019-05-22T02:25:36Z

👋 I just ran into this on a personal project using a .sassrc file to import Primer CSS from node_modules with Parcel. After the expected Primer warnings for breaking changes, Parcel gives an ENOENT error looking for octocat-spinner-16px.gif.

scriptmaster · 2020-05-30T04:05:50Z

cross-platform snippet (win and *nix) to download the missing images. open a cmd or shell and cd to the dir where you want the images folder to be created and download the missing images with this script:

note: windows users should have wget installed via> choco install wget (https://chocolatey.org/)

mkdir images && pushd images
mkdir spinners && pushd spinners

wget https://github.com/images/spinners/octocat-spinner-16px.gif
wget https://github.com/images/spinners/octocat-spinner-32.gif
wget https://github.com/images/spinners/octocat-spinner-32-EAF2F5.gif

popd
mkdir modules && pushd modules
mkdir ajax && pushd ajax

wget https://github.com/images/modules/ajax/success@2x.png
wget https://github.com/images/modules/ajax/error@2x.png
wget https://github.com/images/modules/ajax/success.png
wget https://github.com/images/modules/ajax/error.png

popd && popd && popd

> parcel index.html
Server running at http://localhost:1234
√  Built in 3.09s.

/images/spinners/octocat-spinner-16px.gif
/images/modules/ajax/success.png
/images/modules/ajax/error.png
/images/spinners/octocat-spinner-32.gif
/images/modules/ajax/success@2x.png
/images/modules/ajax/error@2x.png
/images/spinners/octocat-spinner-32-EAF2F5.gif

👋 I just ran into this on a personal project using a .sassrc file to import Primer CSS from node_modules with Parcel. After the expected Primer warnings for breaking changes, Parcel gives an ENOENT error looking for octocat-spinner-16px.gif.

@shawnbot @skullface

beary · 2020-08-22T05:45:34Z

Perhaps the picture references should be removed from the code. As a css library, the ideal situation should be that there is only css in the source code, such as bootstrap. Users who want to use pictures as a background should write this part of the code by themselves.

tony13tv · 2020-08-22T08:57:39Z

cross-platform snippet (win and *nix) to download the missing images. open a cmd or shell and cd to the dir where you want the images folder to be created and download the missing images with this script:

mkdir images && pushd images
mkdir spinners && pushd spinners

curl https://github.com/images/spinners/octocat-spinner-16px.gif -O
curl https://github.com/images/spinners/octocat-spinner-32.gif -O
curl https://github.com/images/spinners/octocat-spinner-32-EAF2F5.gif -O

popd
mkdir modules && pushd modules
mkdir ajax && pushd ajax

curl https://github.com/images/modules/ajax/success@2x.png -O
curl https://github.com/images/modules/ajax/error@2x.png -O
curl https://github.com/images/modules/ajax/success.png -O
curl https://github.com/images/modules/ajax/error.png -O

popd && popd && popd

based on @scriptmaster's code

scriptmaster · 2020-09-21T03:57:23Z

@beary perhaps we should introduce more problems to an unsolved simple/defined problem, by suggesting cross-referenced/inspired solution via text, and not via code or pull-request. Or I'd focus on what I had to with primer css to deliver to the customers.

github-actions · 2021-05-26T22:29:17Z

Hi! This issue has been marked as stale because it has been open with no activity for 180 days. You can comment on the issue or remove the stale label to keep it open. If you do nothing, this issue will be closed in 7 days.

alex-kinokon · 2021-06-22T01:17:14Z

This is not done. Please reopen the issue.

simurai · 2022-02-09T12:10:34Z

FYI: @abdounikarim created a PR #1932 to add the images linked in background-images.

Below a cross-post about the reason why we are trying to move away from using images and an example how you can override the image path if you still like to keep them in your own app/site.

I'm gonna close this (#1932) as a wontfix. Because we now have color modes we are moving to replace those gif and pngs to svg that can adapt to different themes. We also plan to overhaul the Form group validation and probably won't be using background-image: to add icons.

If the current paths don't work for your app, you could override it to your own file structure. For example add the my-app (or your own name) to the <html> element and then the following styles:

.my-app .form-group > .form-group-body {
  .form-control {
    &.is-autocheck-loading {
      background-image: url('../images/spinners/octocat-spinner-16px.gif');
    }

    &.is-autocheck-successful {
      background-image: url('../images/modules/ajax/success.png');
    }

    &.is-autocheck-errored {
      background-image: url('../images/modules/ajax/error.png');
    }
  }
}

zeke · 2022-02-09T17:26:13Z

Thanks for the update @simurai 💛

emwalker · 2022-05-08T19:22:35Z

I've used @primer/css for several years for a low-effort side project, and recently this issue came up when I was attempting to upgrade from Razzle 3 to Razzle 4. I'm a novice when it comes to JavaScript, and I had assumed the error was related to Razzle, due to it coming up in connection with the upgrade. It took me quite a while to think to check whether the images were actually under node_modules/@primer/css. Once I realized they were not there, that got me to looking at @primer/css as the source of the issue rather than Razzle.

emplums added bug 💓collab a vibrant hub of collaboration labels Mar 12, 2019

emplums added the type: enhancement label Mar 12, 2019

shawnbot added the ⭐️rep an industry leading reputation label Mar 12, 2019

yaili added type: bug 🐞 and removed bug labels Feb 27, 2020

simurai mentioned this issue Mar 12, 2020

Accessible form validation #1044

Merged

github-actions bot added the Stale Automatically marked as stale. label May 26, 2021

github-actions bot closed this as completed Jun 2, 2021

abdounikarim mentioned this issue Feb 7, 2022

Correcting missing images #1932

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Host image assets within package #722

Host image assets within package #722

emplums commented Mar 12, 2019 •

edited

Loading

shawnbot commented Mar 12, 2019

zeke commented Mar 12, 2019

shawnbot commented Mar 13, 2019

ocarreterom commented Mar 13, 2019 •

edited

Loading

shawnbot commented Mar 13, 2019

skullface commented May 22, 2019

scriptmaster commented May 30, 2020 •

edited

Loading

beary commented Aug 22, 2020

tony13tv commented Aug 22, 2020

scriptmaster commented Sep 21, 2020 •

edited

Loading

github-actions bot commented May 26, 2021

alex-kinokon commented Jun 22, 2021

simurai commented Feb 9, 2022 •

edited

Loading

zeke commented Feb 9, 2022

emwalker commented May 8, 2022

Host image assets within package #722

Host image assets within package #722

Comments

emplums commented Mar 12, 2019 • edited Loading

shawnbot commented Mar 12, 2019

zeke commented Mar 12, 2019

shawnbot commented Mar 13, 2019

ocarreterom commented Mar 13, 2019 • edited Loading

shawnbot commented Mar 13, 2019

skullface commented May 22, 2019

scriptmaster commented May 30, 2020 • edited Loading

cross-platform snippet (win and *nix) to download the missing images. open a cmd or shell and cd to the dir where you want the images folder to be created and download the missing images with this script:

note: windows users should have wget installed via> choco install wget (https://chocolatey.org/)

beary commented Aug 22, 2020

tony13tv commented Aug 22, 2020

cross-platform snippet (win and *nix) to download the missing images. open a cmd or shell and cd to the dir where you want the images folder to be created and download the missing images with this script:

scriptmaster commented Sep 21, 2020 • edited Loading

github-actions bot commented May 26, 2021

alex-kinokon commented Jun 22, 2021

simurai commented Feb 9, 2022 • edited Loading

zeke commented Feb 9, 2022

emwalker commented May 8, 2022

emplums commented Mar 12, 2019 •

edited

Loading

ocarreterom commented Mar 13, 2019 •

edited

Loading

scriptmaster commented May 30, 2020 •

edited

Loading

scriptmaster commented Sep 21, 2020 •

edited

Loading

simurai commented Feb 9, 2022 •

edited

Loading