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

Add local statics for images and typefaces #3132

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

dbolack-ab
Copy link
Collaborator

@dbolack-ab dbolack-ab commented Nov 8, 2023

This solves issue #1958.

Add static paths /staticImages and /staticFonts

If a local environment is detected ( per existing loginc for login ), the paths are added via the configuration variables hb_fonts and hb_images, respectively.

These should be fully qualified paths.

This solves issue naturalcrit#1958.

Add static paths /staticImages and /staticFonts

If a local environment is detected ( per existing loginc for login )
paths are added using the values in HB_IMAGES and HB_FONTS or the default values of /staticImages and /staticFonts respectively.
@calculuschild calculuschild temporarily deployed to homebrewery-pr-3132 December 5, 2023 03:29 Inactive
@calculuschild calculuschild temporarily deployed to homebrewery-pr-3132 January 17, 2024 17:22 Inactive
@calculuschild calculuschild temporarily deployed to homebrewery-pr-3132 January 18, 2024 20:04 Inactive
@dbolack-ab dbolack-ab self-assigned this Jan 19, 2024
@dbolack-ab dbolack-ab added the 🔍 R0 - Needs first review 👀 PR ready but has not been reviewed label Jan 19, 2024
@Gazook89
Copy link
Collaborator

Gazook89 commented Feb 8, 2024

I only worked with the images part for now, but I couldn't get it to work? This is likely my fault, so I probably just need a little more guidance.

I have pulled your PR here, confirmed that is what I am working with, and did npm install just in case. Then, I added a folder to the root directory .../homebrewery/staticImages and dropped a png in there. For now, I left the /homebrewery/config/defaults.json alone, relying on the built-in default you've added.

In my brew, I did this:

![cat warrior](Class-Table-Frame.png)

![cat warrior](staticImages/Class-Table-Frame.png)

And a couple of other variants. No dice. I then tried adding the hb_images (and HB_IMAGES) key to the default.json to see if I could get it to work that way, again no dice.

I also tried simplifying that line down to:

	app.use(express.static('./staticImages'));

Any tips?

@ericscheid
Copy link
Collaborator

ericscheid commented Feb 8, 2024

![cat warrior](Class-Table-Frame.png)

![cat warrior](staticImages/Class-Table-Frame.png)

Any tips?

Those two url references would be relative to the current page location, and thus expanded to something like https://homebrewery.naturalcrit.com/edit/:id/Class-Table-Frame.png or https://homebrewery.naturalcrit.com/edit/:id/staticImages/Class-Table-Frame.png or
https://homebrewery.naturalcrit.com/share/:id/Class-Table-Frame.png or
https://homebrewery.naturalcrit.com/new/Class-Table-Frame.png.

Try ![cat warrior](/staticImages/Class-Table-Frame.png) to cause the img src to be an absolute url.

Here's an example of another image that should already work:
https://homebrewery.naturalcrit.com/assets/redTriangle.png
![red triangle](/assets/redTriangle.png)

@Gazook89
Copy link
Collaborator

Gazook89 commented Feb 8, 2024

Yeah I thought I tried that as well with no luck but I will try again later today. I had also tried changing the code to match what @G-Ambatte had suggested in the linked issue, and then following that format, to no avail.

@ericscheid
Copy link
Collaborator

What happens if you right-click the broken image and say "open image in new tab" .. what does the window.location look like?

@G-Ambatte
Copy link
Collaborator

I'm getting 406 responses when attempting to get a local image file, which appears to be from the content-negotiation middleware, That said, disabling said middleware does NOT result in the image being correctly served, so I suspect that there's still something that I've missed.

@Gazook89
Copy link
Collaborator

Gazook89 commented Feb 8, 2024

As noted in gitter, when i copy the images to the build folder, it works. see below in edit

In the buildHomebrew.js script:

	// Move assets
	await fs.copy('./themes/fonts', './build/fonts');
	await fs.copy('./themes/assets', './build/assets');
	await fs.copy('./client/icons', './build/icons');
	await fs.copy('./staticImages', './build/staticImages');

(note, this in the "themes" portion, which doesn't quite sit right with me...this is sort of out of my depth, though).

and in app.js:

	// Add Static Local Paths
	app.use(express.static('build/staticImages'));

but it does require including the directory:

![alt](staticImages/image.png)

duh, of course it works...it doesn't even need the app.js bit...because it's just referencing a directory in the build, which isn't anything special. So I guess....if someone is working locally, is there anything particularly wrong with just telling them to create a path ./build/images/ and using that to store their images? Do we need have this in app.js at all?

Sorry, I think i've lost the thread.

@G-Ambatte
Copy link
Collaborator

if someone is working locally, is there anything particularly wrong with just telling them to create a path ./build/images/ and using that to store their images?

fs.emptyDirSync('./build');

This line deletes the contents of /build/ every time the build action runs, so we should not recommend that the users store anything inside that directory.

I suspect that if we follow the recommendation below and use an absolute path to the staticImage directory, we'll resolve the issue without needing to copy the folder to the build directory at all.
image

@dbolack-ab
Copy link
Collaborator Author

As noted in gitter, when i copy the images to the build folder, it works. see below in edit

In the buildHomebrew.js script:

	// Move assets
	await fs.copy('./themes/fonts', './build/fonts');
	await fs.copy('./themes/assets', './build/assets');
	await fs.copy('./client/icons', './build/icons');
	await fs.copy('./staticImages', './build/staticImages');

(note, this in the "themes" portion, which doesn't quite sit right with me...this is sort of out of my depth, though).

and in app.js:

	// Add Static Local Paths
	app.use(express.static('build/staticImages'));

but it does require including the directory:

![alt](staticImages/image.png)

duh, of course it works...it doesn't even need the app.js bit...because it's just referencing a directory in the build, which isn't anything special. So I guess....if someone is working locally, is there anything particularly wrong with just telling them to create a path ./build/images/ and using that to store their images? Do we need have this in app.js at all?

Sorry, I think i've lost the thread.

Sorry, seem to have missed these questions.

My read on the original request was this was desired for situations where the user probably does not want or cannot copy the static content into place or needs to point at a path outside of /build/ such as a mounted docker volume.

I've updated the initial commit comment to be a little more clear on usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔍 R0 - Needs first review 👀 PR ready but has not been reviewed
Projects
Status: Waiting for First Review
Development

Successfully merging this pull request may close these issues.

None yet

6 participants