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

Use AlertsList React component in /alerts_list (try #2) #3481

Conversation

chrisnorwood
Copy link
Contributor

What this PR does

fix for #1803

  • Refactors AlertsList component into seperate CampaignAlerts and AdminAlerts components with shared functionality
  • Allows for asynchronous 'resolving' for AdminAlerts
  • Campaign alerts functionality not changed
  • Sort options are determined from alerts that are actually available to sort

Screenshots

Before:
1803-before

After:
1803-after-NEW

Open questions and concerns

Note 1: CampaignAlerts and AdminAlerts both still using UNSAFE_componentWillMount instead of componentDidMount. I attempted the switch, and I found out the MultiSelectField component would break when given its default values from a Redux state that was dispatched in "DidMount" instead of a "WillMount". As numerous other components rely on MultiSelectField, I think this is better solved in another issue/PR.

Note 2: @ragesoss : You suggested earlier today that the admin view could use the AlertType names instead of the "friendly" versions. In this PR, both campaign and admin alert lists use the friendly name, as computed in the transformAlertsIntoOptions helper function defined in AlertsHandler. I found it easier to have them both work the same. If you strongly prefer that to be different, let me know.

@chrisnorwood chrisnorwood force-pushed the feature/convert-alerts-list-to-react-2 branch from 6874941 to 85ba860 Compare November 13, 2019 22:21
fix for WikiEducationFoundation#1803

* Refactors AlertsList component into seperate CampaignAlerts and AdminAlerts components with shared functionality
* Allows for asynchronous 'resolving' for AdminAlerts
* Campaign alerts functionality not changed
* Sort options determined from available alerts to sort
@chrisnorwood chrisnorwood force-pushed the feature/convert-alerts-list-to-react-2 branch from 85ba860 to b09049a Compare November 13, 2019 23:52
@chrisnorwood
Copy link
Contributor Author

Any thoughts/concerns/changes needed here?

@bwreid
Copy link
Contributor

bwreid commented Nov 21, 2019

Apologies for our slow response on this @chrisnorwood, and that you've had to solve merge request issues consistently. On the whole, this looks great. It is a much better flow for admin.

My only suggestion would be to change the CampaignAlerts file to be an ES6 class if you're comfortable doing so. We are slowly making these changes in the rest of the codebase.

@chrisnorwood
Copy link
Contributor Author

chrisnorwood commented Nov 21, 2019

No worries, @bwreid . Yes, I can absolutely do that for both CampaignAlerts and AdminAlerts actually.

The couple merge conflicts here I resolved with the Github GUI, because on my last PR I tried to do it locally, but ended up muddying up the PR with dozens of other people's commits (if that makes sense?).

If I git pull my origin branch (the one being referenced here in this PR) to my local branch, thus fast forwarding it by the 55 commits or w/e, do you have any tips on how to avoid those 55 commits showing up here in this PR? Do I have to squash them or something? Sorry for the sort of lame question lol, but I'm kind of new to this, and I don't wanna mess it up again.

@bwreid
Copy link
Contributor

bwreid commented Nov 21, 2019

Hey @chrisnorwood . Sorry about the git troubles! Instead of merging, you could likely rebase.

If you want to fix the changes locally, I think the easiest thing to do would be to likely delete your local branch (assuming you have no new commits on there) and then just re-pull this branch.

@chrisnorwood
Copy link
Contributor Author

Excellent suggestion, thanks for the insight. I'll hop on that this evening and force push the new changes with the ES6 classes!

@chrisnorwood chrisnorwood force-pushed the feature/convert-alerts-list-to-react-2 branch from 4c69947 to 145b5c8 Compare November 22, 2019 22:32
@chrisnorwood
Copy link
Contributor Author

Thanks for the tips @bwreid, that worked great.

@bwreid
Copy link
Contributor

bwreid commented Nov 26, 2019

@ragesoss Any additional comments you have on this?

@ragesoss
Copy link
Member

No problems jump out in the code. I'm planning to try it out locally today as a final check.

@ragesoss
Copy link
Member

The unchosen types in the filter select are listed multiple times (if there are multiple instances of that type).

Looks like this affects both the admin list and the campaign list.

multiple filter option repeats

@chrisnorwood
Copy link
Contributor Author

chrisnorwood commented Nov 26, 2019

Oof, my bad for overlooking that. I'll fix that now.

Screen Shot 2019-11-26 at 3 44 00 PM

@ragesoss
Copy link
Member

ragesoss commented Dec 2, 2019

LGTM!

@ragesoss ragesoss merged commit 2d7a513 into WikiEducationFoundation:master Dec 2, 2019
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.

None yet

3 participants