Skip to content

Fix expiring polls not being displayed as such in the WebUI#11835

Merged
Gargron merged 4 commits intomastodon:masterfrom
ClearlyClaire:fixes/poll-expiration
Sep 16, 2019
Merged

Fix expiring polls not being displayed as such in the WebUI#11835
Gargron merged 4 commits intomastodon:masterfrom
ClearlyClaire:fixes/poll-expiration

Conversation

@ClearlyClaire
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Member

@Gargron Gargron left a comment

Choose a reason for hiding this comment

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

This is very likely to run into issues when components are re-used by the renderer and passed completely different props.

@ClearlyClaire
Copy link
Copy Markdown
Contributor Author

Good point (although honestly that's not something that should happen 😩), I'll look into that


state = {
selected: {},
expired: this.props.poll.get('expired') || (new Date(this.props.poll.get('expires_at'))).getTime() < this.props.intl.now(),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Set this to null.

Create _setupTimer method. Call this method from componentDidMount. Call this method from componentWillReceiveProps if nextProps.poll.get('expired') !== this.props.poll.get('expired') etc.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Setting it to null and then changing it in componentDidMount would incur an useless extra render cycle for every poll.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Well, realistically, there's no reason to store expired in state, for example RelativeTimestamp component stores current timestamp in the state, and then the render method takes care of calculating the difference. That's probably the cleanest approach.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'd say there's no reason to store the current timestamp either, tbh.
Changed it to not use the deprecated componentWillReceiveProps and refactor it, it's cleaner imho, and should not do useless renders. However, it will re-fire the timer each time a poll gets updated even if its expiration date does not change. This is infrequent and cheaper than a render, though.

@ClearlyClaire ClearlyClaire force-pushed the fixes/poll-expiration branch 3 times, most recently from 366fa72 to d489216 Compare September 14, 2019 10:43
@Gargron Gargron merged commit 524187b into mastodon:master Sep 16, 2019
hiyuki2578 pushed a commit to ProjectMyosotis/mastodon that referenced this pull request Oct 2, 2019
…#11835)

* Fix expiring polls not being displayed as such in the WebUI

* Reset expiration state and timer when a poll changes

* Refactor timer logic in `_setupTimer`, only set expiration if props have changed

* Refactor and do not use deprecated React lifecycles
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants