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

feat: /data/spark/what-is-spark build #1409

Merged
merged 6 commits into from
Nov 7, 2024

Conversation

mtruj013
Copy link
Contributor

@mtruj013 mtruj013 commented Oct 29, 2024

Done

  • Built page as per doc and design
  • Drive by:
    • Bumped Vanilla and Sass versions (sass bump needed for vanilla)

QA

Issue / Card

Fixes https://warthogs.atlassian.net/browse/WD-15058

@webteam-app
Copy link

@mtruj013 mtruj013 changed the title feat: /data/spark/what-is-spark page feat: /data/spark/what-is-spark build Oct 29, 2024
@eliman11
Copy link
Collaborator

eliman11 commented Oct 30, 2024

Thanks! A couple comments from me:

  • Add alt text for Apache Spark logo in the hero
  • Could you please add the legal copy at the bottom of the page? Sorry I forgot to paste it into the copydoc earlier!
  • "Learn more about OpenSearch" changed into "Learn more about Spark" in the last section

@lyubomir-popov
Copy link
Contributor

Mostly loks good, couple of small things I noticed:

  • p-section--shallow needed around this h2:
image
  • could you please set the width or height of this logo so on mobile it is same size as on desktop?
image
  • I want to sggest an aspect ratio for medium/small screens, but the lazyload div is currently not supported. (If @jmuzina figures out a work around, maybe we can add it). Or we can add it now and it will s tart workign once we fix this bug on the vanilla side:
image

@jmuzina
Copy link
Member

jmuzina commented Oct 30, 2024

We have found that the .lazyloaded div generated by the image template causes the aspect ratio to not be respected. In the short term this can be fixed by applying display: contents to the lazyloaded div:

  .p-image-container,
  [class^='p-image-container--'] {
    // If there is a child element that is not the image, don't let it affect the layout
    & > *:not(.p-image-container__image) {
      display: contents;
    }
}

This will be upstreamed to Vanilla shortly

@mtruj013
Copy link
Contributor Author

@eliman11 about the alt text for the logo hero, can I ask why we're including it? We usually consider these decorative images and the recommended practice is to leave an empty alt text for these so that screen readers will ignore it

@mtruj013
Copy link
Contributor Author

@eliman11 @lyubomir-popov thanks both, I've adressed most of your comments if you could take another look?

Unfortunately the image container snippet is causing some weirdness with the image template. I've added the p-image-container--3-2-on-medium class to the three images regardless so that once the fix is on Vanilla it'll be automatically applied. (cc @jmuzina )

@jmuzina
Copy link
Member

jmuzina commented Oct 30, 2024

Unfortunately the image container snippet is causing some weirdness with the image template. I've added the p-image-container--3-2-on-medium class to the three images regardless so that once the fix is on Vanilla it'll be automatically applied. (cc @jmuzina )

The fix for this has been released in Vanilla version 4.18.1!

@lyubomir-popov
Copy link
Contributor

the mages still havew the 2-3 aspect ratio rather than 3-2 on medium/small
image

@mtruj013
Copy link
Contributor Author

@lyubomir-popov the fix doesn't currently work with the image template. Fix incoming soon though

@eliman11
Copy link
Collaborator

Thanks, looks good to me! Agreed about not including the alt text, thanks for pointing it out.

</div>
</section>

<section class="p-section--deep">
Copy link
Contributor

Choose a reason for hiding this comment

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

High macro potential detected 👀

Copy link
Contributor

@petesfrench petesfrench left a comment

Choose a reason for hiding this comment

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

Looking good! Address the comments and add it to the sitemap and it is good to go

@mtruj013 mtruj013 merged commit 53c57f3 into canonical:feature-spark Nov 7, 2024
8 checks passed
@mtruj013 mtruj013 deleted the what-is-spark branch November 7, 2024 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants