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(tourtip): added leading image #2209

Closed
wants to merge 1 commit into from
Closed

Conversation

ArtBlue
Copy link
Contributor

@ArtBlue ArtBlue commented Nov 15, 2023

Fixes #2178

  • This PR contains CSS changes
  • This PR does not contain CSS changes

Description

This adds a leading container (for future flexibility of possibly using an icon) and an image that goes inside.

Notes

  1. There are still some unanswered questions for DS in the issue. I wanted to get this going so I took my best guess at the answers when building it. However, we still need some final guidance from DS before we move ahead with this.
  2. Adding stories, I realized there was no RTL for tourtip. To add the RTL stories, I had to reorganize the stories a bit (so new Percy baselines will be needed for comparison).
  3. I fixed a minor RTL spacing issue in the footer after the issue got exposed from the RTL stories.

Screenshots

image

Checklist

  • I verify the build is in a non-broken state
  • I verify all changes are within scope of the linked issue
  • I regenerated all CSS files under dist folder
  • I tested the UI in all supported browsers
  • I did a visual regression check of the components impacted by doing a Percy build and approved the build
  • I tested the UI in dark mode and RTL mode
  • I added/updated/removed Storybook coverage as appropriate

@agliga
Copy link
Contributor

agliga commented Nov 17, 2023

This will need an ebayui change.

agliga
agliga previously approved these changes Nov 17, 2023
Copy link
Contributor

@agliga agliga left a comment

Choose a reason for hiding this comment

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

LGTM.
Im not sure what unanswered questions we have but to me is looks fine.

Copy link
Member

@LuLaValva LuLaValva left a comment

Choose a reason for hiding this comment

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

Looks good to me as well

@LuLaValva
Copy link
Member

Punting this to 17.0.0 or until design has resolved issue comments.

@LuLaValva LuLaValva added this to the 17.0.0 milestone Nov 20, 2023
Base automatically changed from 16.9.0 to master November 21, 2023 22:31
@LuLaValva LuLaValva dismissed agliga’s stale review November 21, 2023 22:31

The base branch was changed.

@ArtBlue ArtBlue changed the base branch from master to 17.0.0 November 28, 2023 14:54
@ArtBlue ArtBlue closed this Nov 28, 2023
@ArtBlue ArtBlue deleted the 2178-tourtip-leading-image branch November 28, 2023 19:00
@ArtBlue
Copy link
Contributor Author

ArtBlue commented Nov 28, 2023

Closing this PR in light of the re-evaluation of the issue by design.

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.

3 participants