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

Display images on Adoptions page #20

Merged
merged 13 commits into from
Aug 27, 2024
Merged

Display images on Adoptions page #20

merged 13 commits into from
Aug 27, 2024

Conversation

Dindihub
Copy link
Owner

Display images on Adoption's page to finish setting up the page.

Please review @chalin @lukpueh

Copy link

netlify bot commented Aug 22, 2024

Deploy Preview for tufio ready!

Name Link
🔨 Latest commit ba55bf2
🔍 Latest deploy log https://app.netlify.com/sites/tufio/deploys/66ce59984f1d5d00082a86f3
😎 Deploy Preview https://deploy-preview-20--tufio.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Dindihub Dindihub requested review from lukpueh and chalin August 22, 2024 14:28
Copy link
Collaborator

@chalin chalin left a comment

Choose a reason for hiding this comment

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

This PR will need some rework following the merge of #19. See

for a description of what needs to be done.

Note: instead of putting Adoptions images under static, put them under /content/en/community/adoptions/img.

@Dindihub
Copy link
Owner Author

This PR will need some rework following the merge of #19. See

for a description of what needs to be done.

Note: instead of putting Adoptions images under static, put them under /content/en/community/adoptions/img.

@chalin Okay. I was thinking of opening a separate PR for that but this works too.

@Dindihub
Copy link
Owner Author

This PR will need some rework following the merge of #19. See

for a description of what needs to be done.
Note: instead of putting Adoptions images under static, put them under /content/en/community/adoptions/img.

@chalin Okay. I was thinking of opening a separate PR for that but this works too.

@chalin may I ask why you want the images moved from static to community? It really complicated the code, it wouldn't run. I had to take it back to static :) Not sure what I'm doing wrong.

With the images in static it works just fine.

I tried changing the imgURL on the adoptions.html from {{ $imgUrl := printf "img/logos/adoptions/%s_logo.%s" $img $type | relURL }} to {{ $imgUrl := printf "en/community/adoptions/img/%s_logo.%s" $img $type | relURL }} but it wouldn't work.

@Dindihub Dindihub requested a review from chalin August 24, 2024 12:06
@chalin
Copy link
Collaborator

chalin commented Aug 27, 2024

In #17, I shared with you have to rebase.

Please rebase this PR. You'll need to resolve the conflicts that arise.

Also, can you tell me what output you get when you run git remote -v? If you see upstream, then remove it by running the following command: git remote remove upstream. You won't be contributing to the upstream project, and it just causes headaches otherwise.

@chalin
Copy link
Collaborator

chalin commented Aug 27, 2024

@chalin may I ask why you want the images moved from static to community?

For improved cohesion: the images are only used in the adoptions section.

I tried changing the imgURL on the adoptions.html from {{ $imgUrl := printf "img/logos/adoptions/%s_logo.%s" $img $type | relURL }} to {{ $imgUrl := printf "en/community/adoptions/img/%s_logo.%s" $img $type | relURL }} but it wouldn't work.

After you've moved the files, the path to the images should simply be /img/%s_logo.%s because the path will be local to the folder. The full path you gave above should work too, as long as you drop the en/ prefix (if you look at the preview, there's no en/ in the URL path). Does that help?

@Dindihub
Copy link
Owner Author

In #17, I shared with you have to rebase.

Please rebase this PR. You'll need to resolve the conflicts that arise.

Also, can you tell me what output you get when you run git remote -v? If you see upstream, then remove it by running the following command: git remote remove upstream. You won't be contributing to the upstream project, and it just causes headaches otherwise.

Rebased and removed upstream. I realized my mistake was rebasing on local but forgetting to --force push. I think it's solved now.

@Dindihub
Copy link
Owner Author

@chalin may I ask why you want the images moved from static to community?

For improved cohesion: the images are only used in the adoptions section.

I tried changing the imgURL on the adoptions.html from {{ $imgUrl := printf "img/logos/adoptions/%s_logo.%s" $img $type | relURL }} to {{ $imgUrl := printf "en/community/adoptions/img/%s_logo.%s" $img $type | relURL }} but it wouldn't work.

After you've moved the files, the path to the images should simply be /img/%s_logo.%s because the path will be local to the folder. The full path you gave above should work too, as long as you drop the en/ prefix (if you look at the preview, there's no en/ in the URL path). Does that help?

It works now. Thanks

Signed-off-by: sandra <[email protected]>
Signed-off-by: sandra <[email protected]>
Signed-off-by: sandra <[email protected]>
@Dindihub
Copy link
Owner Author

Please review again @chalin . Thanks

Copy link
Collaborator

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Getting closer, but the images are still not rendering. Run npm test locally and you'll see the invalid links.

For the sake of making progress on this PR, I'll address the issues in a followup commit over this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename this to index.md (note the lack of underscore); otherwise we end up with this stutter in the path:

image

content/en/community/adoptions/img/logos/agl-logo.png Outdated Show resolved Hide resolved
content/en/_index.md Outdated Show resolved Hide resolved
content/en/docs/get-started/adopter.md Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

This page shouldn't exist under /get-started/, but we'll address this separately. I'll file an issue.

layouts/shortcodes/adoptions.html Outdated Show resolved Hide resolved
Copy link
Collaborator

@chalin chalin left a comment

Choose a reason for hiding this comment

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

LGTM! All tests are passing (locally and via GH actions), and the images are rendering correctly in the Adoptions page.

@chalin
Copy link
Collaborator

chalin commented Aug 27, 2024

Merging. @lukpueh if you have any comments we can address them via a followup PR.

@chalin chalin merged commit 46f6058 into main Aug 27, 2024
6 checks passed
@chalin chalin deleted the adoptions branch August 27, 2024 22:59
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