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

Stickerpacks #6242

Merged
merged 29 commits into from Mar 29, 2018

Conversation

@rxl881
Copy link
Contributor

rxl881 commented Feb 23, 2018

Stickerpack related UI

@rxl881 rxl881 requested a review from ara4n Feb 23, 2018
@rxl881 rxl881 assigned dbkr and unassigned ara4n Mar 22, 2018
}

componentWillUnmount() {
window.removeEventListener("resize", this.resize.bind(this));

This comment has been minimized.

Copy link
@dbkr

dbkr Mar 22, 2018

Member

You'll probably find this doesn't work since the new bound function you're creating is not equal to the one you created previously.

This comment has been minimized.

Copy link
@rxl881

rxl881 Mar 26, 2018

Author Contributor

It seems to work, but thanks very much for pointing this out (makes a lot of sense not that you've mentioned it), I've moved the binding to the constructor.

import React from 'react';
import PropTypes from 'prop-types';

export default class GenericElementContextMenu extends React.Component {

This comment has been minimized.

Copy link
@dbkr

dbkr Mar 22, 2018

Member

A brief comment on what this does might be useful. It appears to tell a child element about window resizes, but I'm not sure what that has to do with context menus.

This comment has been minimized.

Copy link
@rxl881

rxl881 Mar 26, 2018

Author Contributor

The position of the contextual menus are set at construction time. If you resize the window while the menu is visible the element that the menu is linked to moves but the menu does not as it is displayed in a DOM element on the document root. By adding a resize listener the menu parent can be triggered to update the position or (in this case) close the menu.

this.state={
parent,
parentBox: parent.getBoundingClientRect(),
};

This comment has been minimized.

Copy link
@dbkr

dbkr Mar 22, 2018

Member

These don't need to be part of the state, in fact this will cause un unnecessary render after the first one because the state's changing.

@dbkr dbkr assigned rxl881 and unassigned dbkr Mar 22, 2018
@rxl881

This comment has been minimized.

Copy link
Contributor Author

rxl881 commented Mar 26, 2018

Thanks @dbkr - Hopefully those points are addressed now. Can you please take another look?

Cheers.

@rxl881 rxl881 assigned dbkr and unassigned rxl881 Mar 26, 2018
@dbkr
dbkr approved these changes Mar 26, 2018
@rxl881 rxl881 merged commit 1abe577 into develop Mar 29, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@rxl881 rxl881 deleted the rxl881/snapshot branch Mar 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.