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

refactor: article graph to use functional component #5498

Merged

Conversation

justiceotuya
Copy link
Contributor

@justiceotuya justiceotuya commented Oct 4, 2023

NOTE: Please review the pull request process before opening your first PR: https://github.com/WikiEducationFoundation/WikiEduDashboard/blob/master/CONTRIBUTING.md#pull-request-process

What this PR does

refactor: article_graph class component to use functional component from this Issue #5393

Screenshots

Screen.Recording.2023-10-04.at.10.56.09.PM.mov

@Aminehassou
Copy link
Contributor

Could you add a reference to the relevant issue in your PR? (it makes it easier to track which components have been refactored if each PR references back to issue #5393)

@justiceotuya
Copy link
Contributor Author

Could you add a reference to the relevant issue in your PR? (it makes it easier to track which components have been refactored if each PR references back to issue #5393)

done

@TheTrio
Copy link
Contributor

TheTrio commented Oct 19, 2023

@justiceotuya could you resolve conversations as you go along addressing them? Makes it easier to track changes

@justiceotuya
Copy link
Contributor Author

@justiceotuya could you resolve conversations as you go along addressing them? Makes it easier to track changes

Resolved and fixed @TheTrio

Comment on lines 125 to 126
if (dataIncludesWp10 || !articleData) {
editSize = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not required

Copy link
Contributor

Choose a reason for hiding this comment

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

same here — this hasn’t been fixed and yet the conversation is marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unresolved and fixed @TheTrio

Comment on lines 93 to 94
if (!dataIncludesWp10 || !articleData) {
radioInput = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

this isn't required either

Copy link
Contributor

Choose a reason for hiding this comment

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

why is this marked as resolved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unresolved and fixed @TheTrio

Copy link
Contributor

@TheTrio TheTrio left a comment

Choose a reason for hiding this comment

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

the PR looks fine, I'll test it manually once tomorrow.

@justiceotuya justiceotuya requested a review from TheTrio October 21, 2023 16:02
Comment on lines 91 to 94
let radioInput = (
<div>
<div className="input-row">
<input
Copy link
Contributor

Choose a reason for hiding this comment

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

missing a check for articleData[0].wp10

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TheTrio This has been fixed

Copy link
Contributor

@TheTrio TheTrio left a comment

Choose a reason for hiding this comment

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

LGTM

@ragesoss ragesoss merged commit c4b7bea into WikiEducationFoundation:master Oct 26, 2023
1 check passed
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.

4 participants