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

JS Code Review #42

Merged
merged 33 commits into from
Jan 26, 2021
Merged

JS Code Review #42

merged 33 commits into from
Jan 26, 2021

Conversation

tfrommen
Copy link
Contributor

@tfrommen tfrommen commented Jan 7, 2021

This PR targets JavaScript-specific code improvements or changes to be made that have been mentioned in #38.

In more detail, these include:

  • Move type definitions to dedicated file.
  • Move arrayMove function to dedicated folder and file.
  • Move SortableMultiValueElement component to dedicated file.
  • Move plugin data into dedicated file.
  • Import WordPress dependencies from their respective package, not the wp global.
  • Move HOCs for authors select from index/plugin to component file.
  • Remove unused parameter, type import, and documentation.
  • Use ReactElement type instead of JSX.Element.
  • Rename authors select component file to match component name.
  • Move sortable select container to dedicated file.
  • Move static props out of component.
  • Export container HTML class for reuse.
  • Add placeholder for select.
  • Simplify SCSS selector.
  • Add stylelint exception for overriding an inline style.

This PR conflicts with #41 in that both make changes to the package.json file. If you want to merge both PRs, you should be able to manually accept all changes—make sure to keep the most recent package version. If you do not or only partially merge this PR, you will have to adapt some JavaScript/TypeScript tooling configuration! This PR would currently fail on CI—iff ESLint would get executed—because it does not include linting configuration changes the production code assumes to exist. This is done in #41.

@johnbillion
Copy link
Member

Thanks for this Thorsten, this is very helpful and I agree with all the changes 👍 . I'll respond to your questions in #38 after I've finished checking the other PRs.

@johnbillion johnbillion merged commit 3b20d9c into develop Jan 26, 2021
@tfrommen tfrommen mentioned this pull request Jan 26, 2021
@johnbillion johnbillion deleted the js-review branch January 26, 2021 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants