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.

Sign up for GitHub

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

Jump to bottom

SUL23-480: caption missing but really under the image #217

Merged

pookmish merged 3 commits into 1.x from SUL23-480--banner-caption

Sep 18, 2024

Contributor

jenbreese commented Sep 17, 2024 •

edited

Loading

READY FOR REVIEW

Summary

The caption was on the page but hidden under the image. I didn't verify it wasn't on the page.
Possibly remove the fragment added?

Review By (Date)

9/18

Criticality

Normal

Urgency

Normal

Review Tasks

Setup tasks and/or behavior to test

Navigate to /news/stanford-libraries-make-nuremberg-international-military-tribunal-trial-archives-1945-1946
Verify there is a visible caption
Review the solution.

Site Configuration Sync

Is there a config:export in this PR that changes the config sync directory?

Front End Validation

Design is approved by @ user?
HTML validation: Is the markup using the appropriate semantic tags and passes validation? Or, QA request ticket created?
Cross-browser testing: Has been performed? Or, QA request ticket created?
Automated accessibility: Scans performed? Or, QA request ticket created?
Manual accessibility: Manually tested? Or, QA request ticket created?

Backend / Functional Validation

Code

Are the naming conventions following our standards?
Does the code have sufficient inline comments?
Is there anything in this code that would be hidden or hard to discover through the UI?
Are there any code smells?
Are tests provided? eg (unit, behat, or codeception)

Code security

Are all forms properly sanitized?
Any obvious security flaws or new areas for attack?

General

Is there anything included in this PR that is not related to the problem it is trying to solve?
Is the approach to the problem appropriate?

Affected Projects or Products

Does this PR impact any particular projects, products, or modules?

Associated Issues and/or People

Resources

AMP Tool
Accessibility Manual Test Script
HTML Validator
Browserstack and link to Browserstack Credentials


          Removed the absolute

74ab918

vercel bot commented Sep 17, 2024 •

edited

Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
su-library	✅ Ready (Inspect)	Visit Preview	Sep 17, 2024 7:48pm

github-actions bot added the size/xs label

jenbreese requested a review from pookmish

September 17, 2024 14:46

pookmish requested changes

View reviewed changes

src/components/node/stanford-news/page-display.tsx Outdated

                     {imageUrl && (
                       <figure className="relative mx-auto mb-100 aspect-[16/9] lg:w-10/12">
-                        <Image className="object-cover" src={buildUrl(imageUrl).toString()} alt={imageAlt || ""} fill />
+                        <Image className="!relative object-cover" src={buildUrl(imageUrl).toString()} alt={imageAlt || ""} fill />

Contributor

pookmish Sep 17, 2024

I would suggest staying away from !important uses, especially on the image component. NextJS Image component applies it's own styles that make it function the way it does for responsive sizes. My suggestion is to adjust the wrapper a little bit.:

<figure className="table">
  <span className="relative aspect-[16/9]">
     <Image .../>
  </span>
  <figcaption className="table-caption">...</figcaption>
</figure>

The <Image> component needs the immediate parent to be relative positioned. So changing this markup keeps that relative positioning but takes the caption out of it so the caption can be its own thing. You'll also notice the table classes. This can help with the positioning of the caption.

Contributor Author

jenbreese Sep 17, 2024

Thank you. That's a good idea with the table class.

Contributor Author

jenbreese Sep 17, 2024

@pookmish I used caption instead of table-caption because of how it displays with a table.

jenbreese assigned pookmish


          fixup to table

d67c502

vercel bot deployed to Preview

September 17, 2024 16:20

View deployment

pookmish requested changes

View reviewed changes

src/components/node/stanford-news/page-display.tsx Outdated

-                      <figure className="relative mx-auto mb-100 aspect-[16/9] lg:w-10/12">
-                        <Image className="object-cover" src={buildUrl(imageUrl).toString()} alt={imageAlt || ""} fill />
+                      <figure className="mb-100 table">

Contributor

pookmish Sep 17, 2024

Suggested change

      
                    <figure className="mb-100 table">
          
                    <figure className="mb-100 table w-full">

src/components/node/stanford-news/page-display.tsx Outdated

+                      <figure className="mb-100 table">
+                        <span className="relative mx-auto block aspect-[16/9] lg:w-10/12">
+                          <Image className="object-cover" src={buildUrl(imageUrl).toString()} alt={imageAlt || ""} fill />
+                        </span>
                         {node.suNewsBannerMediaCaption && (
                           <figcaption className="caption text-center">{node.suNewsBannerMediaCaption}</figcaption>

Contributor

pookmish Sep 17, 2024

Suggested change

      
                        <figcaption className="caption text-center">{node.suNewsBannerMediaCaption}</figcaption>
          
                        <figcaption className="table-caption caption-bottom text-center">{node.suNewsBannerMediaCaption}</figcaption>

src/components/node/stanford-news/page-display.tsx Outdated

                         {node.suNewsBannerMediaCaption && (
                           <figcaption className="caption text-center">{node.suNewsBannerMediaCaption}</figcaption>
                         )}
                       </figure>
                     )}
                     {node.suNewsBanner?.__typename === "MediaVideo" && (
-                      <figure className="relative mx-auto mb-100 aspect-[16/9] w-10/12">
-                        <Oembed url={node.suNewsBanner.mediaOembedVideo} />
+                      <figure className="mb-100 table">

Contributor

pookmish Sep 17, 2024

Suggested change

      
                    <figure className="mb-100 table">
          
                    <figure className="mb-100 table w-full">

src/components/node/stanford-news/page-display.tsx Outdated

+                      <figure className="mb-100 table">
+                        <span className="relative mx-auto block aspect-[16/9] w-10/12">
+                          <Oembed url={node.suNewsBanner.mediaOembedVideo} />
+                        </span>
                         {node.suNewsBannerMediaCaption && (
                           <figcaption className="caption text-center">{node.suNewsBannerMediaCaption}</figcaption>

Contributor

pookmish Sep 17, 2024

Suggested change

      
                        <figcaption className="caption text-center">{node.suNewsBannerMediaCaption}</figcaption>
          
                        <figcaption className="table-caption caption-bottom text-center">{node.suNewsBannerMediaCaption}</figcaption>


          fixup

dfd1264

vercel bot deployed to Preview

September 17, 2024 19:48

View deployment

pookmish approved these changes

View reviewed changes

pookmish merged commit 297d719 into 1.x

5 checks passed

pookmish deleted the SUL23-480--banner-caption branch

September 18, 2024 16:21

imonroe added a commit that referenced this pull request


          Release 1.4.1 (#219)

4a77208

* SUL23-620: Corrected shared tags events view contextual filter argument name

* SUL23-593: Show list paragraph empty message if no results

* Add Image credit field on media image

* SUL23-480: caption missing but really under the image (#217)

* Removed the absolute

* fixup to table

* fixup

* Updated dependencies

* SUL23-481 | hardcode h3 headings for feature collections (#218)

* SUL23-481 | hardcode headings for feature collections

* adjust heading level for collections

* Improved invalidation when comparing the home page path

* Updated version in package.json

---------

Co-authored-by: Mike Decker <[email protected]>
Co-authored-by: Jen Breese <[email protected]>
Co-authored-by: Rebecca Hong <[email protected]>

imonroe added a commit that referenced this pull request


          release 1.4.2 (#220)

ea72821

* SUL23-620: Corrected shared tags events view contextual filter argument name

* SUL23-593: Show list paragraph empty message if no results

* Add Image credit field on media image

* SUL23-480: caption missing but really under the image (#217)

* Removed the absolute

* fixup to table

* fixup

* Updated dependencies

* SUL23-481 | hardcode h3 headings for feature collections (#218)

* SUL23-481 | hardcode headings for feature collections

* adjust heading level for collections

* Improved invalidation when comparing the home page path

---------

Co-authored-by: Mike Decker <[email protected]>
Co-authored-by: Jen Breese <[email protected]>
Co-authored-by: Rebecca Hong <[email protected]>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels