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

[Dialog] Incorrect paddingTop when opening Dialog with dynamic content #1676

Closed
grimor opened this issue Sep 18, 2015 · 30 comments · Fixed by lunohq/luno-web#2
Closed

[Dialog] Incorrect paddingTop when opening Dialog with dynamic content #1676

grimor opened this issue Sep 18, 2015 · 30 comments · Fixed by lunohq/luno-web#2
Labels
bug 🐛 Something doesn't work

Comments

@grimor
Copy link

grimor commented Sep 18, 2015

When I have structure like that

<Dialog ref="mediaModal" actions={standardActions} title="Multimedia Browser" autoDetectWindowHeight={true} autoScrollBodyContent={false} contentClassName="media-modal" contentStyle={{ width: '90%', maxWidth: 'auto', height: '100%' }} contentInnerStyle={{ maxHeight: '100%' }}>
    <div className="media-modal__menu">
      <List subheader="Media types">
        <ListItem primaryText="Images" leftIcon={<FontIcon className="material-icons">image</FontIcon>} />
        <ListItem primaryText="Audio" leftIcon={<FontIcon className="material-icons">library_music</FontIcon>} />
        <ListItem primaryText="Videos" leftIcon={<FontIcon className="material-icons">video_library</FontIcon>} />
        <ListItem primaryText="3D" leftIcon={<FontIcon className="material-icons">blur_on</FontIcon>} />
      </List>
    </div>
    <div className="media-modal__content">
      <MediaList items={items}/>
    </div>
</Dialog>

Where MediaList contain grid of images. It gets paddingTop calculated incorrectly Padding on first opening

But, on second _positionUpdate() call it have proper positioning. On update

I suppose that images get calculated their dimensions later than modal (I've set width and height properties on <img> tag)

I'll try to work on PR on that, but maybe someone is smarter than me :)

@oliviertassinari
Copy link
Member

I'm experiencing a similar issue. The first time I render a Dialog paddingTop is correct, but if the component update, paddingTop is incorrect.

@aslauris
Copy link

I'm facing the same issue. You can use window.dispatchEvent(new Event('resize')) as a workaround (call it after appending the content). It triggers window resize event and forces dialog position/size to be recalculated. Hope it will be helpful.

@Ben07
Copy link

Ben07 commented Nov 12, 2015

@aslauris your solution is nice,but when I show images in a dialog,I don't know when to call the dispatchEvent function because of load time of image.has any idea?

@rhythnic
Copy link

@Ben07 you can probably set call window.dispatchEvent from inside the 'componentWillUpdate' lifecycle callback. i assume that gets called as the image loads, but I'm not sure.

@Ben07
Copy link

Ben07 commented Nov 13, 2015

@rhythnic thanks,I set call window.dispatchEvent inside the 'componentDidUpdate' and use DIV tag which has Immutability height to wrap these images.It's works.

@alitaheri alitaheri added the bug 🐛 Something doesn't work label Dec 8, 2015
@alitaheri alitaheri modified the milestone: 0.14.0 Release Dec 9, 2015
@oliviertassinari oliviertassinari removed this from the 0.14.0 Release milestone Dec 22, 2015
@morenoh149
Copy link

Any advice on solving this? I'd be happy to submit a PR if given direction.

@echenley
Copy link
Contributor

@morenoh149 you can fix it by overriding the dialog styles: #2322 (comment).

There's not really a perfect solution at the moment. You could also try something like this:

<Dialog
  contentStyle={ styles.dialogContent }
  bodyStyle={ styles.dialogBody }
  style={ styles.dialogRoot }
  repositionOnUpdate={ false }
/>

var styles = {
  dialogRoot: {
    display: 'flex',
    alignItems: 'center',
    justifyContent: 'center',
    paddingTop: 0
  },
  dialogContent: {
    position: 'relative',
    width: '90%',
    maxWidth: 500
  },
  dialogBody: {
    paddingBottom: 0
  }
};

@tintin1343
Copy link
Contributor

@morenoh149 : were you able to solve the issue with the above suggestion? Let me know if that works

@tintin1343
Copy link
Contributor

@grimor : was this issue resolved for you?

@morenoh149
Copy link

haven't had a chance. sorry.

@tintin1343 tintin1343 assigned tintin1343 and unassigned tintin1343 Apr 21, 2016
@oliviertassinari
Copy link
Member

@tintin1343 I'm still experiencing a similaire issue with dialog and dynamic content.

@tintin1343
Copy link
Contributor

tintin1343 commented Apr 22, 2016

@oliviertassinari : I can look into it. Could you post your findings here? I can take it over. I actually made a change in the dialog PaddingTop attribute in the DatePicker PR i told you about.

@nathanmarks
Copy link
Member

@oliviertassinari @tintin1343 FYI guys I am currently experimenting with some 0.16.0 dialog updates.

@tintin1343
Copy link
Contributor

@oliviertassinari : Was this fixed with the PR?

@oliviertassinari
Copy link
Member

@tintin1343 No, I was referring to a related issue.

@ayw818
Copy link

ayw818 commented Jun 3, 2016

@echenley thanks for #1676 (comment).

I had to do 90vw instead of 90% to get it to work though. Thank you for a great workaround.

dialogContent: {
    position: 'relative',
    width: '90vw',
    maxWidth: 500
  },

@gre
Copy link

gre commented Aug 1, 2016

can we just have a prop to disable the vertical center?
I really don't want some of my dialogs to y offset like crazy because inner content changes.

@gre
Copy link

gre commented Aug 1, 2016

Forking @echenley's ideas:

import React, {
  Component,
} from "react";

import Dialog from "material-ui/Dialog";

const styles = {
  dialogRoot: {
    display: "flex",
    alignItems: "center",
    justifyContent: "center",
    paddingTop: 0
  },
  dialogContent: {
    position: "relative",
    width: "80vw",
    transform: "",
  },
  dialogBody: {
    paddingBottom: 0
  }
};

export default class CustomDialog extends Component {
  render () {
    const { ...props } = this.props;
    return (
      <Dialog
        {...props}
        contentStyle={ styles.dialogContent }
        bodyStyle={ styles.dialogBody }
        style={ styles.dialogRoot }
        repositionOnUpdate={ false }
      />
    );
  }
}

I'm not sure why this is not the default.

@jbbae
Copy link

jbbae commented Aug 4, 2016

Guys, I'm trying to pass in the "styles.dialogRoot" into the Dialog component with just a "paddingTop" of 0, and it just doesn't seem to get applied into the Dialog. I'm doing exactly the same steps as above, just applying the paddingTop. Any ideas why style isn't working?

@gre
Copy link

gre commented Aug 4, 2016

Yeah same actually, paddingTop does not seem to get reset...
We need a simple way to "center in the middle" dynamically and without it to be handled by JS.. should be handled by CSS..

I really enjoy almost all components of material-ui but this one is really limited..

@gre
Copy link

gre commented Aug 4, 2016

@nickbae91 my guess is you use repositionOnUpdate but you shouldn't.
i've updated my code to move the {...props} above so it's always getting set

@gre
Copy link

gre commented Aug 4, 2016

see here: https://github.com/callemall/material-ui/blob/master/src/Dialog/Dialog.js#L218 the padding get re-set in repositionOnUpdate case

@GRiMe2D
Copy link

GRiMe2D commented Nov 2, 2016

This is somehow related to animations. One can manually trigger resize event, but Dialog will repositioned after animations.

Currently, my workaround is this. But you'll notice how Dialog repositioned after animation

componentDidUpdate: function () {
        setTimeout(() => {
                window.dispatchEvent(new Event('resize'));
        }, 500);
},

@nightlyop
Copy link

I think the dialog could be centered vertically by using flexbox patterns withouth calculating padding-top. However, I fixed it with

<Dialog className: 'dialog-root' ... > ... </Dialog>
.dialog-root {
  padding-top: 0 !important;
}

Doing the same using inline styles does not work. Is it overwritten by the calculated value?

Because my dialog has so much content that it shouldn't have any top-padding, this is good enough as a workaround until this bug is fixed.

@oliviertassinari
Copy link
Member

That issue has been solved on the next branch. I'm closing it.

@lcaixeta-fixeads
Copy link

I still have this issue on v0.17.0 could you please confirm this fix for this version? thanks.

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 25, 2017

The issue is still present on the 0.x.x releases. We don't have the resources to maintained two versions. If the master branch is affected, we close issues once they are fixed on the next branch.

@kamran-koupayi
Copy link

You can use this props for Dialog
repositionOnUpdate={true}

@nightlyop
Copy link

@kamran-koupayi: repositionOnUpdate does not work in my case. The child (content) of the dialog is updated but the dialog does not detect it.

@djbuckley
Copy link

djbuckley commented Jul 17, 2018

I've just fixed an issue with our application where this was happening.
The basic issue is that a dialog with a child control has no clue that the child has updated.

My fix is along these lines (no point in showing you the code its exceptionally specific to our situation)

in the dialog code :
<childControl updated={() => {this.setState({childUpdated: new Date()}) />

in the child :
componentDidUpdate() {
if (this.state.oldData !== this.state.newData) {
this.setState({oldData: this.state.newData});
if (this.props.updated) { this.props.updated(); }
}
}

... the theory is that when the child updates its state data, it fires the callback which updates the parent, but that only happens when the specific data I need watching changes.
If you don't have some way to check your data has changed before the callback fires, you end up in an infinite loop of render, state change, callback

hope thats useful to someone....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work
Projects
None yet
Development

Successfully merging a pull request may close this issue.