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(popover): introduce disabled flag #2229

Closed
wants to merge 4 commits into from

Conversation

medinarkhov
Copy link
Contributor

Fixes #2188

PR created as a continuation of PR #2217

[disabled] would be a useful feature for popover\toltip, allowing user to create conditional popover\tooltip.
<div [ngbPopover]="It can be conditionally hidden!" [disabled]="hidePopover"></div>

@medinarkhov
Copy link
Contributor Author

@pkozlowski-opensource please review. If the change looks good then I will add another commit to update:

  • tooltip to have [disabled] flag (remove conditional content then?)
  • update API
  • update demo

Copy link
Member

@maxokorokov maxokorokov left a comment

Choose a reason for hiding this comment

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

We should find a better name, otherwise LGTM.

I think it's a better option, than not showing it for falsy values (of what? content? title?) and more user-friendly than using imperative API

Would have the same API for the tooltip

P.S. Also afterwards, please open separate PRs, not commits for the tooltip (it will be a breaking change I think) and demos updates.

/**
* A flag indicating if a given popover is disabled and should not be displayed.
*/
@Input() disabled = false;
Copy link
Member

Choose a reason for hiding this comment

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

  1. Don't think it will be acceptable, too many collisions, ex:
<button [disabled]="true" ngbPopover>...</button>

maybe [showPopover] or [disablePopover]?

<div [ngbPopover]="text" [showPopover]="text !== ''" />
<div [ngbPopover]="text" [disablePopover]="text === ''" />

Don't like [hidePopover], because it implies that popover is open at the first place

  1. All other inputs initial values come from NgbPopoverConfig. I think we should put this one there as well. Not 100% sure though if there is a use case for massively initializing all popovers as disabled... So this is questionable

@medinarkhov
Copy link
Contributor Author

@maxokorokov thank you for prompt review.

Update:

  1. renamed [disabled] flag to [disablePopover]
  2. updated popover-config.ts
  3. won't touch Tooltip in this PR

API page looks good. Let me know if more changes required? Would you like demo example?

Copy link
Member

@maxokorokov maxokorokov left a comment

Choose a reason for hiding this comment

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

LGTM except for one small comment, and let's wait for feedback from @pkozlowski-opensource, if he agrees to add this to public API

/**
* A flag indicating if a given popover is disabled and should not be displayed.
*/
@Input() disablePopover = false;
Copy link
Member

Choose a reason for hiding this comment

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

You just forgot this.disablePopover = config.disablePopover in the constructor, and no need to initialize it here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch! fixed)

@maxokorokov maxokorokov added this to the 1.1.0 milestone Mar 21, 2018
@pkozlowski-opensource
Copy link
Member

LGTM, thnx!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants