Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Component refactor and loading indicator stub #11
Component refactor and loading indicator stub #11
Changes from all commits
44a3579
74b94b6
65b8466
7e847e8
5ed7d84
10aa23c
ea777b6
1246eeb
b7632b3
6452be0
601abc5
899b9cf
17614fb
4c44bf6
bcd7f7f
0e84c0f
88599f8
0a6cdeb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked closely, but does this need any kind of early return, to make sure we're not hitting the API every time this re-renders? It seems like React triggers a lot of re-renders (sometimes even multiple within a few milliseconds), and you have to be careful when hooking into it, to make sure you're not doing a lot of extra work and causing performance issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only when the component re-mounts. I don't think that will happen, as it is the root component, but I'm not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big deal, but it might be nice to move this to a separate file, like
components/main-view.js
, to separate the view from the controller. I think that's a common pattern in React.Here's an example of what I mean:
https://github.com/iandunn/compassionate-comments/tree/b8f1690/admin/main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true. I'm a fan of separation of concerns. This leaves some room for future improvements!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might need to use
Fragment
here, because support for<>
was only recently added to G, and I don't think it'll be available until WP 5.3 is out. If you tested this in 5.2 and it worked, though, then it's fine to keep.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be supported, as it was implemented in React 16.2. As far as I can see, it has been in WP Core for quite some time. I'm on 5.2 and it works without a problem.
I find this to be less confusing than having a "Fragment" component. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the native
sort()
function may be able to replace this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used the original code (by @avillegasn ?). I haven't looked at the internals, maybe something to look into in the future.