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

Fix PropTypes Error Create New Post Page #14969

Merged
merged 1 commit into from Oct 11, 2021
Merged

Fix PropTypes Error Create New Post Page #14969

merged 1 commit into from Oct 11, 2021

Conversation

papuruth
Copy link
Contributor

@papuruth papuruth commented Oct 6, 2021

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

Fixes PropTypes error in Create new post page

Related Tickets & Documents

#14837

Added/updated tests?

  • Yes
  • No, as because existing tests will suffice.
  • I need help with writing tests

[optional] What gif best describes this PR or how it makes you feel?

docker

@papuruth papuruth requested review from a team, nickytonline and Ridhwana and removed request for a team October 6, 2021 18:27
@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Oct 6, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2021

Thank you for opening this PR! We appreciate you!

For all pull requests coming from third-party forks we will need to
review the PR before we can process it through our CI pipelines.

A Forem Team member will review this contribution and get back to
you as soon as possible!

@rhymes rhymes requested review from aitchiss and rhymes and removed request for nickytonline and Ridhwana October 7, 2021 10:18
Copy link
Contributor

@aitchiss aitchiss left a comment

Choose a reason for hiding this comment

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

Nice work!

I've left a couple of suggestions, but they're not blocking. I also noticed that if you're a member of an organization, there are still a few PropTypes errors being logged out on this page.

It can always be tackled separately, so I'll leave it up to you whether to tackle those errors here too.

Thanks for tidying up these errors! 🧹 ✨

@@ -65,7 +65,7 @@ export class ArticleForm extends Component {
};

static defaultProps = {
organizations: '',
organizations: '[]',
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is needed - I can't see any error being flagged for the current default prop?

@@ -24,7 +24,7 @@ export const ErrorList = ({ errors }) => {
};

ErrorList.propTypes = {
errors: PropTypes.objectOf(PropTypes.string).isRequired,
errors: PropTypes.object.isRequired,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
errors: PropTypes.object.isRequired,
errors: PropTypes.objectOf(PropTypes.arrayOf(PropTypes.string)).isRequired

suggestion (not blocking) - we could make these even more specific, since we know the object values should all be arrays of strings

* @param {boolean} props.previewShowing Boolean to decide if to show the preview
* @param {string} props.helpFor Section for which help is shown
* @param {string} props.helpPosition Offset from the top of the help component
* @param {number} props.helpPosition Offset from the top of the help component
Copy link
Contributor

Choose a reason for hiding this comment

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

🔥

@pr-triage pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 8, 2021
@benhalpern benhalpern merged commit 33742e7 into forem:main Oct 11, 2021
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Oct 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants