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.

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

feat(hero): image start #1828

Merged
merged 24 commits into from
Nov 7, 2023
Merged

feat(hero): image start #1828

merged 24 commits into from
Nov 7, 2023

Conversation

monicawheeler
Copy link
Collaborator

@monicawheeler monicawheeler commented Oct 23, 2023 •
edited
Loading

Description

Add the "left" image option for the Hero called "Image start".

Additionally, I optimized images in the `assets/images" directory.

Screenshots

Tech Before After After 2
React before - react .after - react1 after - react2
Rails before - rails .after - rails N/A

Testing in sage-lib

  1. Navigate to http://localhost:4000/pages/component/hero?tab=preview
  2. Verify the Rails version is looking and working as expected
  3. Navigate to the react version by selecting the option for "React Component" to ensure the linking works
  4. Verify the react version is looking and working as expected

Testing in kajabi-products

  1. (LOW) Added an image start prop. Since that is new and other changes are documentation only, this is not a breaking change.

Related

DSS-476

@monicawheeler monicawheeler self-assigned this Oct 23, 2023
@ju-Skinner ju-Skinner added the WIP label Oct 24, 2023
@monicawheeler monicawheeler force-pushed the feat/hero-image-start branch 2 times, most recently from a392903 to ab6dac9 Compare October 30, 2023 20:02
@monicawheeler monicawheeler added documentation Improvements or additions to documentation and removed WIP labels Oct 30, 2023
@monicawheeler monicawheeler marked this pull request as ready for review October 30, 2023 20:39
@monicawheeler monicawheeler requested a review from a team October 30, 2023 20:39
@ju-Skinner ju-Skinner changed the title Feat/hero image start feat(hero): image start Oct 30, 2023
ju-Skinner
ju-Skinner requested changes Oct 30, 2023
View reviewed changes
Copy link
Collaborator

@ju-Skinner ju-Skinner left a comment

Choose a reason for hiding this comment

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

Looks great overall..One small fix in the Hero.jsx file.

  • Set default prop value to false instead of null.

packages/sage-react/lib/Hero/Hero.jsx Outdated Show resolved Hide resolved
pixelflips
pixelflips requested changes Oct 30, 2023
View reviewed changes
Copy link
Member

@pixelflips pixelflips left a comment

Choose a reason for hiding this comment

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

A few comments to note a few small issues.

NOTE:
Non-blocking, but any thoughts on the story naming, just to help keep the sidebar a bit more tidy?

Something like the following might work ok since they are the major ones that aren't used in every instance. No strong preference here, but just a thought so feel free to ignore.

  • Sizes
  • Borderless
  • Contained
  • Custom Background
  • Image Start
  • CTA Attribute

packages/sage-assets/lib/stylesheets/components/_hero.scss Outdated Show resolved Hide resolved
packages/sage-react/lib/Hero/Hero.story.mdx Outdated Show resolved Hide resolved
packages/sage-react/lib/Hero/Hero.story.mdx Outdated
src: `${heroFull}`,
alt: 'Multi layered illustration of UI elements and woman clasping her hands together in excitement'
}}
ctaAttributes={{
Copy link
Member

Choose a reason for hiding this comment

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

Not entirely sure what the issue is, but I am getting a related JS error:

Screenshot 2023-10-30 at 2 05 17 PM

Note: it may not be this exact instance, but seems related to ctaAttributes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pixelflips - I took a look, fiddled with the proptypes, but having trouble clearing this. I'll take a look tomorrow!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Huge thanks to @ju-Skinner for helping me solve this issue and get this out the door! 🙌🏻

packages/sage-react/lib/Hero/Hero.story.mdx Show resolved Hide resolved
packages/sage-react/lib/Hero/Hero.story.mdx Outdated Show resolved Hide resolved
pixelflips
pixelflips reviewed Oct 30, 2023
View reviewed changes
Copy link
Member

@pixelflips pixelflips left a comment

Choose a reason for hiding this comment

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

Also noticed the React Component link on the Docs site will need to be updated as it currently points to default (/docs/sage-hero--default) which is no longer there.

@pixelflips pixelflips requested a review from a team October 30, 2023 21:40
@monicawheeler
Copy link
Collaborator Author

monicawheeler commented Oct 30, 2023 •
edited
Loading

Looks great overall..One small fix in the Hero.jsx file.

* Set default prop value to `false` instead of `null`.

@ju-Skinner Sorry but which prop? or line?

@monicawheeler
Copy link
Collaborator Author

Also noticed the React Component link on the Docs site will need to be updated as it currently points to default (/docs/sage-hero--default) which is no longer there.

@pixelflips - this should also be resolved now that you have the current commits! Let me know if not!

@monicawheeler
Copy link
Collaborator Author

A few comments to note a few small issues.

NOTE: Non-blocking, but any thoughts on the story naming, just to help keep the sidebar a bit more tidy?

Something like the following might work ok since they are the major ones that aren't used in every instance. No strong preference here, but just a thought so feel free to ignore.

* Sizes

* Borderless

* Contained

* Custom Background

* Image Start

* CTA Attribute

@pixelflips - thank you for the feedback and thoughts! I think this organization makes more sense and I'll update both React and Rails to align.

@pixelflips
Copy link
Member

Also noticed the React Component link on the Docs site will need to be updated as it currently points to default (/docs/sage-hero--default) which is no longer there.

@pixelflips - this should also be resolved now that you have the current commits! Let me know if not!

@monicawheeler Yup, good to go. Not sure how I didn't have the latest, but sorry for all the confusion.

ju-Skinner
ju-Skinner approved these changes Oct 31, 2023
View reviewed changes
Copy link
Collaborator

@ju-Skinner ju-Skinner left a comment

Choose a reason for hiding this comment

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

Great work.

@ju-Skinner ju-Skinner requested a review from pixelflips November 6, 2023 15:29
@monicawheeler
docs(hero): react - add mdx base file
24f6b18
@monicawheeler
docs(hero): react - add images to mdx and create first story setup
ea8104c
@monicawheeler
docs(hero): react - work out examples for contained images
de010a2
@monicawheeler
docs(hero): react - add prop definitions
e7f08a3
@monicawheeler
docs(hero): react - update when to use and not to use sections
a20d24b
@monicawheeler
docs(hero): react - get the examples to render like rails
56a8190
@monicawheeler
docs(hero): rails - update the alt text to match image
b04ba6e
@monicawheeler
docs(hero): react and rails - update descriptions and output react pr…
456c24d 
…op table
@monicawheeler
docs(hero): rails - update props, rules do, rules dont
322a258
@monicawheeler
docs(hero): react - set bool default values to other than null
daff8ca
@monicawheeler
docs(hero): scss - update breakpoint to use variable
d9bf716
@monicawheeler
docs(hero): react - add borderless prop to the examples
4ee4219
@monicawheeler
docs(hero): rails - update naming of visual examples
f196a7c
@monicawheeler
docs(hero): update outline of documentation
c914040
@monicawheeler
fix(hero): react - correct ctaAttribute prop type
66f5daf
pixelflips
pixelflips approved these changes Nov 7, 2023
View reviewed changes
Copy link
Member

@pixelflips pixelflips left a comment

Choose a reason for hiding this comment

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

Great work! LGTM! 👍🏼

@monicawheeler monicawheeler merged commit 6dafbb3 into develop Nov 7, 2023
4 checks passed
@monicawheeler monicawheeler deleted the feat/hero-image-start branch November 7, 2023 16:58
@QuintonJason QuintonJason mentioned this pull request Nov 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ju-Skinner ju-Skinner ju-Skinner approved these changes

@pixelflips pixelflips pixelflips approved these changes

Assignees

@monicawheeler monicawheeler

Labels
documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@monicawheeler @pixelflips @ju-Skinner