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 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 left a comment

looking good so far. i like

src/Popover.js Show resolved Hide resolved
bpas247 added 5 commits May 21, 2019
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
@bpas247

This comment has been minimized.

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

bpas247 added 2 commits May 21, 2019
this should be applied based on the user's preference, via className
@taion
taion approved these changes May 24, 2019
Copy link
Member

taion left a comment

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/PopoverTriggerBehaviors.js Outdated Show resolved Hide resolved
@taion

This comment has been minimized.

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
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>
@taion
taion approved these changes May 31, 2019
@bpas247

This comment has been minimized.

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 Jun 21, 2019
@jquense jquense merged commit f608c42 into master Jun 21, 2019
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@jquense jquense changed the title Modulate Popover Components feat: modulate Popover Components Jun 21, 2019
@jquense jquense deleted the modulate-popover-components branch Jun 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.