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(app-shell): to add label to maintenance page layouts #1583

Merged
merged 5 commits into from
Jun 19, 2020

Conversation

tdeekens
Copy link
Contributor

Summary

This pull request adds a label which is used on the images alt to all maintenance pages.

Description

Our maintenance pages mostly have: title, two paragraphs and an image. The image itself however doesn't contain any alt text. It would be good to mostly use the title as the alt text on the image.

This also eases querying for the image with byRole('img', { name: //i }) under test.

I had multiple solution in mind when working on this:

  1. Add a optional string of label which goes to the img alt. The img alt needs to be a string and can't be React.Node
  2. Just use the title and pass it as the alt. Slighly less flexible but less noisy for consumers. However, the title is an React.Node as it's a FormattedMessage while the alt needs a string.
  3. Refactor everything to just accept *IntlMessage for instance titleIntlMessage. This however is a breaking change to the public component.
  4. Offer two sets of props: a bit too much work for a little thing like this

@changeset-bot
Copy link

changeset-bot bot commented Jun 18, 2020

🦋 Changeset is good to go

Latest commit: 32aed78

We got this.

This PR includes changesets to release 6 packages
Name Type
@commercetools-frontend/application-components Minor
@commercetools-frontend/application-shell Minor
merchant-center-application-template-starter Patch
playground Patch
@commercetools-local/visual-testing-app Patch
@commercetools-website/components-playground Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Jun 18, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/commercetools/merchant-center-application-kit/6g3ggk79g
✅ Preview: https://merchant-center-a-git-feat-add-label-to-maintenance-page-abab4e.commercetools.vercel.app

@tdeekens tdeekens self-assigned this Jun 18, 2020
@tdeekens tdeekens added 🙏 Status: Dev Review Waiting for technical reviews 🚀 Type: New Feature Something new labels Jun 18, 2020
@vercel vercel bot temporarily deployed to Preview June 18, 2020 16:02 Inactive
Copy link
Member

@ahmehri ahmehri left a comment

Choose a reason for hiding this comment

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

Thanks!

@vercel vercel bot temporarily deployed to Preview June 19, 2020 07:17 Inactive
@vercel vercel bot temporarily deployed to Preview June 19, 2020 08:44 Inactive
@tdeekens tdeekens requested a review from Rombelirk June 19, 2020 08:51
@tdeekens tdeekens force-pushed the feat/add-label-to-maintenance-page-layout branch from 98d594a to 2d6b7a1 Compare June 19, 2020 09:02
@tdeekens tdeekens removed the request for review from YahiaElTai June 19, 2020 09:10
@vercel vercel bot temporarily deployed to Preview June 19, 2020 11:02 Inactive
@tdeekens tdeekens added the 🚀 Status: ship it Triggers an merge if rules match via bot label Jun 19, 2020
@kodiakhq kodiakhq bot merged commit 053ae10 into master Jun 19, 2020
@kodiakhq kodiakhq bot deleted the feat/add-label-to-maintenance-page-layout branch June 19, 2020 11:19
This was referenced Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙏 Status: Dev Review Waiting for technical reviews 🚀 Status: ship it Triggers an merge if rules match via bot 🚀 Type: New Feature Something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants