-
Notifications
You must be signed in to change notification settings - Fork 33
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
(#4414) NCIDS 2 Column Updates - Image & Video #4467
Conversation
6ae2c92
to
a4f7a79
Compare
ODE DeploymentCode has been deployed to ODE 838. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Images and videos should be within a <figure>
element using <figcaption>
for the caption. I am going to call in @olitharp-nci on this on what we should explicitly do. E.g. is it something like:
<figure class="nci-figure">
<img class="nci-image">
<figcaption class="nci-figure__caption">
<span class="nci-figure__source">...
</figcaption>
Or,
The following where we have some figure mixins.
<figure class="nci-image">
</figure>
OR,
<figure class="nci-figure nci-figure--image">
</figure>
Figures are used all over for Images, Videos, Tables. Technically MDN even suggests figures for pull quotes.
a4f7a79
to
17b479c
Compare
17b479c
to
2d9e00a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall comments so I don't have to write them for each aspect ratio:
- The cgov_image display modes should not be named
ncids_aspect_ratio_X_Y
.- Once again the
ncids_image
paragraph name is a little vague, but whatever. - This is going to suck a but it should follow what we have been doing for other components.
- The labels should be
Image Display: NCIDS Image: XxY Crop
- The machine names should be
ncids_image_ncids_image_XxY
.- Yes, I see
ncids_image
is there twice, but it is following what we did for Promo, Feature Cards and Flag Cards.
- Yes, I see
- The labels should be
- Once again the
- You need different image styles for each of the renderings. Do not reuse
ncids_featured
orncids_promo
. You should have your own set of display modes and they need to set the max width to whatever the appropriate max width is for these use cases. The image style is for the presentation use of that image, not some sort of generic thing.
docroot/profiles/custom/cgov_site/modules/custom/cgov_home_landing/cgov_home_landing.module
Outdated
Show resolved
Hide resolved
...v_image/config/install/core.entity_view_display.media.cgov_image.ncids_aspect_ratio_16_9.yml
Outdated
Show resolved
Hide resolved
2d9e00a
to
468be4a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more items
...ustom/cgov_site/themes/custom/ncids_trans/front-end/src/lib/components/cgdp-video/index.scss
Outdated
Show resolved
Hide resolved
...ustom/cgov_site/themes/custom/ncids_trans/front-end/src/lib/components/cgdp-video/index.scss
Outdated
Show resolved
Hide resolved
...rofiles/custom/cgov_site/themes/custom/ncids_trans/templates/content/image/image_macros.twig
Outdated
Show resolved
Hide resolved
.../custom/cgov_site/themes/custom/ncids_trans/front-end/src/lib/components/cgdp-video/index.ts
Outdated
Show resolved
Hide resolved
a7ce339
to
689c2cb
Compare
2fffd26
to
5585a69
Compare
@dev-rana-publicis @dlescarbeau
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requesting changes for me to make - but also requirements.
...custom/cgov_home_landing/config/install/field.storage.paragraph.field_image_aspect_ratio.yml
Outdated
Show resolved
Hide resolved
docroot/profiles/custom/cgov_site/modules/custom/cgov_home_landing/cgov_home_landing.module
Outdated
Show resolved
Hide resolved
...rofiles/custom/cgov_site/themes/custom/ncids_trans/templates/content/image/image_macros.twig
Outdated
Show resolved
Hide resolved
...rofiles/custom/cgov_site/themes/custom/ncids_trans/templates/content/image/image_macros.twig
Outdated
Show resolved
Hide resolved
|
097d511
to
ad8b761
Compare
Per client feedback on 10/24:
|
2e6e9b1
to
6c7d121
Compare
Federal team has approved this PR - reviewed by Anna and Alex on 10/29 |
6c7d121
to
eb9f1c6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passes product review.
Closes #4414
ODE: https://ncigovcdode838.prod.acquia-sites.com/