[Feature] Improve DataBrowser relation viewer #446
Conversation
|
By analyzing the blame information on this pull request, we identified @drew-gross, @TylerBrock and @gavrix to be potential reviewers. |
ghost
added
| let queryFilters = JSON.parse(this.props.location.query.filters); | ||
| queryFilters.forEach((filter) => { | ||
| filters = filters.push(new Map(filter)); | ||
| }); | ||
| } | ||
|
|
||
| const { className, entityId, relationName } = this.props.params; |
There was a problem hiding this comment.
Is that a re-declaration here?
There was a problem hiding this comment.
I was just blinded for shortcut. Remove above?
There was a problem hiding this comment.
If above is not used yes :)
There was a problem hiding this comment.
Can I force-push to fix it?
There was a problem hiding this comment.
No need to force push, we squash upon merge.
|
The overall code look great, and the feature is 💯. I'm no react expert but it seems we're bloating the Browser component with a lot of corner case Logic for the relations. Can we compose or inherit instead? What do you think @peterdotjs @drew-gross |
|
@flovilmart please make it better. and above mistakes will not happen if eslint and flow applied to this project... have a plan? |
|
I'm asking for advice here. Want to make sure we keep the code base maintainable. Otherwise I'm impressed! 👍 |
|
I want to submit a PR which makes this project more maintainable but this project is too huge to patch this myself. There are too many legacies... |
| import Field from 'components/Field/Field.react'; | ||
| import Label from 'components/Label/Label.react'; | ||
| import TextInput from 'components/TextInput/TextInput.react'; | ||
| import { List, Map } from 'immutable'; |
There was a problem hiding this comment.
This component doesn't need immutable structures. Please remove this before squashing.
There was a problem hiding this comment.
That's your code :) you should do the changes
|
@beingbook updated the pull request. |
|
@beingbook @flovilmart generally we'll prefer composition over inheritance but in this case Browser already has several states for various dialogs that it seems okay to keep it in the same component but may be worth looking at in a future refactor PR as it gets larger. RE: flow and eslint, we had discussed around the time it was open sourced but there wasn't enough request for it to justify the refactor. For future maintainability I totally agree that it would be beneficial. eslint will be a sizable task to refactor but with flow we can go with the approach of any new file that is touched to be flowified. If you're interested in taking this on we're totally supportive of it. Looks great overall though! |
| <span className={styles.details}> | ||
| {props.details} | ||
| </span> | ||
| const goBack = () => history.goBack(); |
There was a problem hiding this comment.
will this functionality be any different than hitting browser back?
There was a problem hiding this comment.
I know it matters. I just added this because relation viewer has url statement. If a user access relation viewer first, it will matter. But I have no idea to implement this better. Can you suggest about it?
|
@beingbook updated the pull request. |
1 similar comment
|
@beingbook updated the pull request. |
|
what's next step? |
|
@beingbook I haven't had a chance to patch it and try it out but looks good otherwise. I'll update once I've gotten a chance to test it out. |
|
@beingbook you're in good hands with @peterdotjs! |
| @@ -0,0 +1,98 @@ | |||
| import * as Filters from 'lib/Filters'; | |||
There was a problem hiding this comment.
it is imported but not used.
|
@beingbook updated the pull request. |
|
oops. I did something wrong... I guess rebase and force push made ref tree twisted... |
|
@beingbook updated the pull request. |
1 similar comment
|
@beingbook updated the pull request. |
|
@beingbook updated the pull request. |
relation viewer