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

amitha/feature/dialog-is-reference-hidden #322

Merged
merged 4 commits into from
Nov 4, 2021

Conversation

amit-hanoch
Copy link
Contributor

@amit-hanoch amit-hanoch commented Nov 4, 2021

Hide dialog when reference is hidden

Updating existing component

Basic

  • PR has description
  • Changes to the component are backward compatible (including selectors structure). If not - add to the title of the PR "BREAKABLE_CHANGE""
  • All changes to the component are reflected in the ReadMe
  • If component is old and was not compliant with the latest guidelines - it was fixed (optional)

Style

Storybook

  • All the changes to the component should be reflected in the Storybook.
  • Component passed Accessibility Plugin checks

Tests

  • All current tests are passing
  • New functionality is covered with tests
  • Tests are compliant with TESTING_README.md instructions

@orrgottlieb orrgottlieb merged commit 207624d into master Nov 4, 2021
@orrgottlieb orrgottlieb deleted the amitha/feature/dialog-is-reference-hidden branch November 4, 2021 13:40
return (
<DialogContent
isReferenceHidden={hideWhenReferenceHidden && isReferenceHidden}
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe send isReferenceHidden as is
and also pass the other prop (hideWhenReferenceHidden) to the dialogContent component?

@@ -346,12 +348,19 @@ export default class Dialog extends PureComponent {
...modifiers
]}
>
{({ placement, style, ref, arrowProps }) => {
{({ placement, style, ref, arrowProps, isReferenceHidden }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

@orrgottlieb are we ok with sending new objects/arrays every time to the popper component?

Copy link
Contributor

Choose a reason for hiding this comment

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

it is the render props methodology, it is a render function. isReferenceHidden is a boolean @MosheZemah

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant the code above that line (that wasn't changed)

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.

4 participants