Skip to content

Feat/featur highlight extras #506

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

Merged
merged 5 commits into from
Aug 22, 2019
Merged

Feat/featur highlight extras #506

merged 5 commits into from
Aug 22, 2019

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.

2 participants