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

fix: (core) Add Additional class to popover component, introduce nested popover example #1646

Merged
merged 3 commits into from
Jan 2, 2020

Conversation

JKMarkowski
Copy link
Contributor

@JKMarkowski JKMarkowski commented Nov 21, 2019

Please provide a link to the associated issue.

fixes: #1633

Please provide a brief summary of this pull request.

There is added new property on popover, to add custom classlist for popover container. It was missing mostly, when user wanted to put popover inside popover, to differentiate them.
I added also interesting use case with nested-popover, as example.

Please check whether the PR fulfills the following requirements

Documentation checklist:

@JKMarkowski JKMarkowski added the core Core library specific issues label Nov 21, 2019
@JKMarkowski JKMarkowski added this to the sprint 24 - Maester Aemon milestone Nov 21, 2019
@JKMarkowski JKMarkowski added this to In progress in Development via automation Nov 21, 2019
@netlify
Copy link

netlify bot commented Nov 21, 2019

Deploy preview for fundamental-ngx ready!

Built with commit 02ce757

https://deploy-preview-1646--fundamental-ngx.netlify.com

@JKMarkowski JKMarkowski changed the title fix: (core) Add Additional class to popover component fix: (core) Add Additional class to popover component, introduce nested popover example Nov 22, 2019
@JKMarkowski JKMarkowski force-pushed the fix/1633-popover-additional-class branch from 5d43e4a to a9cf053 Compare November 22, 2019 12:53
@InnaAtanasova
Copy link
Contributor

To be honest I don't know how I feel about this nested popover...
In terms of code it looks good. About the look... I think we need some design before releasing this.
@droshev what do you think about this?

Screen Shot 2019-11-25 at 2 54 24 PM

Development automation moved this from In progress to Review in progress Nov 25, 2019
Copy link
Contributor

@InnaAtanasova InnaAtanasova left a comment

Choose a reason for hiding this comment

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

I think we need design for this.

@JKMarkowski
Copy link
Contributor Author

JKMarkowski commented Nov 26, 2019

Hi @InnaAtanasova, this is just to show, how user can inject classes to element.

@droshev
Copy link
Contributor

droshev commented Nov 30, 2019

I agree with @InnaAtanasova. It looks odd the way it appears.

@JKMarkowski JKMarkowski force-pushed the fix/1633-popover-additional-class branch from a9cf053 to b2d9616 Compare December 2, 2019 16:32
@JKMarkowski JKMarkowski force-pushed the fix/1633-popover-additional-class branch from b2d9616 to 973d565 Compare December 3, 2019 09:40
@JKMarkowski
Copy link
Contributor Author

Nested class example is removed.

@@ -756,7 +756,7 @@ import { SelectTypesExampleComponent } from './component-docs/select/examples/se
NotificationContentComponent,
DatePickerComplexI18nExampleComponent,
DatetimePickerComplexI18nExampleComponent,
PopoverDropdownComponent
PopoverDropdownComponent,
Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary comma

Development automation moved this from Review in progress to Reviewer approved Dec 30, 2019
@JKMarkowski JKMarkowski force-pushed the fix/1633-popover-additional-class branch from 973d565 to 02ce757 Compare January 2, 2020 12:14
@JKMarkowski JKMarkowski merged commit 3bdf3f7 into master Jan 2, 2020
Development automation moved this from Reviewer approved to Done Jan 2, 2020
@JKMarkowski JKMarkowski deleted the fix/1633-popover-additional-class branch January 2, 2020 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core library specific issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nested fd-popover element inherits predecessor's styles
4 participants