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: modulate Popover Components #3811

Merged
merged 10 commits into from
Jun 21, 2019
Merged

feat: modulate Popover Components #3811

merged 10 commits into from
Jun 21, 2019

Conversation

bpas247
Copy link
Member

@bpas247 bpas247 commented May 21, 2019

This should allow users more customization regarding the Popover component.

Fixes #2449.

start work on decoupling and modulating the popover components
to their own respective components. This will allow the users
to choose how they want the popovers to be displayed, whether they
want a title, no title, or if they want to inject their own
custom title.

also start work on changing the popover docs to
reflect this new behavior.

also start work on updating the popover unit tests to reflect this
new behavior
Copy link
Member

@taion taion left a comment

Choose a reason for hiding this comment

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

looking good so far. i like

src/Popover.js Show resolved Hide resolved
this issue was caused by the HOC function not carrying over the
properties of the function, which were the modulated popover
components.
add a shorthand syntax for declaring a popover with no title.
also fix bsPrefix issues with PopoverTitle and PopoverContent
the actual children prop

i don't know how i missed this 👀
@bpas247 bpas247 marked this pull request as ready for review May 21, 2019 19:56
@bpas247
Copy link
Member Author

bpas247 commented May 21, 2019

So I'm a bit confused on what I should do for the h3 tag found here

bsPrefix = useBootstrapPrefix(bsPrefix, 'popover-header h3');

Should I just remove it and have the users inject their own tag via className prop?

EDIT: this is what I ended up doing in 7efa7df

Copy link
Member

@taion taion left a comment

Choose a reason for hiding this comment

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

This is looking really nice!

src/PopoverContent.js Outdated Show resolved Hide resolved
src/PopoverTitle.js Outdated Show resolved Hide resolved
www/src/examples/Overlays/PopoverBasic.js Outdated Show resolved Hide resolved
www/src/examples/Overlays/PopoverContained.js Outdated Show resolved Hide resolved
www/src/examples/Overlays/PopoverPositioned.js Outdated Show resolved Hide resolved
www/src/examples/Overlays/PopoverPositionedScrolling.js Outdated Show resolved Hide resolved
www/src/examples/Overlays/PopoverTriggerBehaviors.js Outdated Show resolved Hide resolved
@taion
Copy link
Member

taion commented May 24, 2019

Actually, the as thing is only really important for the title (though it's probably correct elsewhere too). It's just nice to be able to match the upstream examples in making the titles <h3>s.

bpas247 and others added 2 commits May 31, 2019 01:29
this will allow users to define a custom element type for their
Popover components (such as setting PopoverTitle to use an 'h3'
element)
Update Popover examples to utilize new `as` prop on `PopoverTitle`

Co-Authored-By: Jimmy Jia <tesrin@gmail.com>
@bpas247
Copy link
Member Author

bpas247 commented Jun 20, 2019

Is there anything keeping this PR from being merged?

@taion taion requested review from jquense and mxschmitt June 21, 2019 02:36
@jquense jquense merged commit f608c42 into master Jun 21, 2019
@jquense jquense changed the title Modulate Popover Components feat: modulate Popover Components Jun 21, 2019
@jquense jquense deleted the modulate-popover-components branch June 21, 2019 12:41
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.

Remove h3 from popover title
3 participants