Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

GH-7236: Migrate edit_post to redux #173

Merged
merged 2 commits into from
Nov 9, 2017
Merged

GH-7236: Migrate edit_post to redux #173

merged 2 commits into from
Nov 9, 2017

Conversation

jespino
Copy link
Member

@jespino jespino commented Oct 20, 2017 •
edited
Loading

Github issue: mattermost/mattermost#7236

Checklist

  • Added or updated unit tests (required for all new features)
  • Has redux changes (please link)

@jasonblais jasonblais added the 2: Dev Review Requires review by a core commiter label Oct 20, 2017
@jespino
Copy link
Member Author

jespino commented Oct 20, 2017

This is ready, but I want to add the tests.

@jasonblais jasonblais added Work in Progress Not yet ready for review and removed 2: Dev Review Requires review by a core commiter labels Oct 20, 2017
@jasonblais
Copy link
Contributor

Sounds good, I added a work in progress label for now

@jespino jespino force-pushed the GH-7236 branch 2 times, most recently from 87ec18a to e0de7b7 Compare October 20, 2017 17:05
@jespino jespino force-pushed the GH-7236 branch 13 times, most recently from b207c73 to 81f3590 Compare November 6, 2017 19:01
@jespino
Copy link
Member Author

jespino commented Nov 6, 2017

Now must be ready to review

@jasonblais jasonblais added 2: Dev Review Requires review by a core commiter and removed Work in Progress Not yet ready for review labels Nov 6, 2017
jwilander
jwilander reviewed Nov 7, 2017
View reviewed changes
Copy link
Member

@jwilander jwilander left a comment

Choose a reason for hiding this comment

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

Really good changes overall, just a few comments

components/edit_post_modal/edit_post_modal.jsx Outdated
}
});
}
this.props.actions.setEditingPost('', 0, '', '');
Copy link
Member

Choose a reason for hiding this comment

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

0/5, maybe set defaults on the action so you can just do this.props.actions.setEditingPost() here

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I agree, the default values could be no-post.

components/edit_post_modal/edit_post_modal.jsx Outdated
const refocusId = this.props.editingPost.refocusId;
if (refocusId !== '') {
setTimeout(() => {
const element = $(refocusId).get(0);
Copy link
Member

Choose a reason for hiding this comment

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

Can we use document.getElementById() instead of jquery? That we can remove jquery from this entire file

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure! 😄

stores/message_history_store.jsx
@@ -1,55 +1,43 @@
// Copyright (c) 2016-present Mattermost, Inc. All Rights Reserved.
Copy link
Member

Choose a reason for hiding this comment

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

I assume this file is kept around because other components depend on it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, exactly the CreatePost and CreateComment components. After migrate that components this file can be removed.

@saturninoabril
Copy link
Member

@jespino This is huge! Looks good to me.

Could you add more tests to increase test coverage? Right now it's a bit low at ~30%.

screen shot 2017-11-07 at 11 32 23 pm

@jespino jespino force-pushed the GH-7236 branch 2 times, most recently from 88328e0 to 763a734 Compare November 7, 2017 20:08
jwilander
jwilander approved these changes Nov 7, 2017
View reviewed changes
Copy link
Member

@jwilander jwilander left a comment

Choose a reason for hiding this comment

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

Great work on this 👍

@jespino
Copy link
Member Author

jespino commented Nov 8, 2017

Thank you! I'm still working on improving the coverage 😄

@jespino jespino force-pushed the GH-7236 branch 2 times, most recently from 5641f79 to 0666ffb Compare November 9, 2017 06:19
@jespino
Copy link
Member Author

jespino commented Nov 9, 2017

Now have a better coverage. @saturninoabril if you find any bad pattern in the tests, let me know and I can change it 😃 (and learn from it).

saturninoabril
saturninoabril approved these changes Nov 9, 2017
View reviewed changes
Copy link
Member

@saturninoabril saturninoabril left a comment

Choose a reason for hiding this comment

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

Honestly, it's impressive, such a great unit test pattern!

@saturninoabril saturninoabril added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Nov 9, 2017
@saturninoabril saturninoabril added this to the v4.5.0 milestone Nov 9, 2017
@saturninoabril saturninoabril merged commit 6bf5525 into mattermost:master Nov 9, 2017
@jespino
Copy link
Member Author

jespino commented Nov 9, 2017

Thank you! 😊

@jespino jespino deleted the GH-7236 branch November 9, 2017 09:37
@lindalumitchell lindalumitchell added the Tests/Not Needed Does not require new release tests label Nov 14, 2017
@jasonblais jasonblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Jan 4, 2018
hmhealey pushed a commit that referenced this pull request Aug 28, 2020
@jwilander @hmhealey
Fix null channel set causing JS error (#173)
2cfe112
hmhealey pushed a commit that referenced this pull request Mar 17, 2021
@jwilander @enahum
Fix null channel set causing JS error (#173)
4b21570
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jwilander jwilander jwilander approved these changes

@saturninoabril saturninoabril saturninoabril approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation Tests/Not Needed Does not require new release tests
Projects
None yet
Milestone
v4.5.0
5 participants
@jespino @jasonblais @saturninoabril @jwilander @lindalumitchell