Skip to content

Conversation

@Inbal-Tish
Copy link
Collaborator

Adding borderRadius, headline and pageControlProps props.

/**
* The headline to be displayed above the title
*/
headline: PropTypes.string,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is very specific to what Growth team wants, why not allowing styling the title and the message so thy can achieve the typography they want?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Your call. You want me to change it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The point was to control the text design though

Copy link
Collaborator

@ethanshar ethanshar Aug 22, 2019

Choose a reason for hiding this comment

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

yea, but it will never good enough, especially if it's an open source component.
someone will always want to customize it
I think enforcement of styling should be more in the private lib than the public one

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok. I'll change it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So you think they will never use the three of them together?

Copy link
Collaborator

Choose a reason for hiding this comment

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

if the idea was to add a third type of content so people can have 3 different contents then I guess this prop make sense
if we're just adding another content prop with the exact same styling of the title prop just so people can render it under the message then it's weird, if that's the case it makes more sense to have custom styling over the those content

Since the current requirement was to still have two contents just in a different order I assume letting them customize the style feels more right

@Inbal-Tish Inbal-Tish requested a review from ethanshar August 21, 2019 10:49
@ethanshar ethanshar merged commit 4b39cab into master Aug 22, 2019
@Inbal-Tish Inbal-Tish deleted the feat/FeaturHighlight_extras branch August 22, 2019 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants