Refactor/term view #21
Conversation
|
Ping @robertabcd for reviewing |
|
@robertabcd done rebasing. Could you consider adding me to the collaborators of your repo after turning on "Protected branches on |
|
Yes, I'm just thinking about that. I only disabled force-push and deletion on dev branch. You can just push there. But if you'd like it to be reviewed, you can still send create PR. |
|
I don't think GitHub has the mode for forcing all changes via Pull Requests even for collaborators? (if it does, please let me know). Anyways, I'll still create PRs for reviewing since it's easier to track changes :) |
src/components/ImagePreviewer.js
Outdated
| export const of = src => Promise.resolve({ src }); | ||
|
|
||
| export const makeSrcResolver = ({ src }) => | ||
| resolvers.reduceRight((acc, resolver) => { |
There was a problem hiding this comment.
I feel like this is a bit showing off. I prefer something like
resolvers.find(r => r.test(src)).request(src)
which expresses the intention better.
You can use Array#unshift to make it insert at front.
src/js/term_ui.js
Outdated
| return ReactDOM.render( | ||
| <Screen | ||
| lines={lines} | ||
| forceWidth={forceWidth} | ||
| showsLinkPreviews={showsLinkPreviews} | ||
| enableLinkInlinePreview={showsLinkPreviews} | ||
| enableLinkHoverPreview={enablePicPreview} |
There was a problem hiding this comment.
Can you change to just use enableLinkInlinePreview and enableLinkHoverPreview in this function only? I think this is better.
There was a problem hiding this comment.
Not sure what you mean here. Did you mean to align the prop names with the local variable names?
There was a problem hiding this comment.
I mean the prop names are good, so just use them as function parameter names. But just change the names in this function, no need to change the whole project.
There was a problem hiding this comment.
That's what I thought. Thanks!
src/components/ImagePreviewer.js
Outdated
|
|
||
| const register = resolvers.push.bind(resolvers); | ||
|
|
||
| register({ |
There was a problem hiding this comment.
Can you add one line of comment before each register on which this resolver is for?
src/components/ImagePreviewer.js
Outdated
|
|
||
| export const of = src => Promise.resolve({ src }); | ||
|
|
||
| export const makeSrcResolver = ({ src }) => |
There was a problem hiding this comment.
Maybe just use the verb resolve, it returns a Promise that does the resolving.
There was a problem hiding this comment.
Renamed to resolveSrcToImageUrl
| height: img.height | ||
| }); | ||
| img.onerror = reject; | ||
| img.src = src; |
There was a problem hiding this comment.
Is this to preload image or to make sure it's a image? I think it should also do a simple check like in prePicRel before sending a request. We don't want to load every link user hovers.
There was a problem hiding this comment.
Just preload image.
The non-parsable links are now handled in promise reject case.
There was a problem hiding this comment.
I realized that prePicRel is no longer called in any places so I removed it (correct me if I'm wrong).
There was a problem hiding this comment.
I see. Call to that function was removed in 8618472, so it currently only loads whitelisted sites. It might be good to resurrect that back later. It doesn't have to be this PR.
There was a problem hiding this comment.
I see. It's used here:
s0 += '<a scol="'+col+'" class="y q'+this.deffg+' b'+this.defbg+'" href="' +ch.getFullURL() + '"' + this.prePicRel( ch.getFullURL()) + ' rel="noreferrer" target="_blank" srow="'+row+'">';But I can't really tell why we need prePicRel with type=? on the anchor tag. Mind to open an issue with more detailed description instead?
There was a problem hiding this comment.
To my understanding, it's used to annotate that the link is an image. Later, the it uses css selector to find all the images and does further processing:
Line 788 in e317cae
There was a problem hiding this comment.
Yep. Created #25
src/components/ImagePreviewer.js
Outdated
| return 20; | ||
| }; | ||
|
|
||
| ImagePreviewer.renderOnHover = ({ left, top, src, height }) => ( |
There was a problem hiding this comment.
Maybe move the two branch out. So this could be
{src ? <Image {...props}> : <Spinner {...props}>}
There was a problem hiding this comment.
Yeah but again, I don't meant to divide React component extensively at the moment. Just want to keep it as simple as possible and do the refactoring in the future.
src/components/ImagePreviewer.js
Outdated
| }; | ||
|
|
||
| render() { | ||
| return this.props.render({ |
There was a problem hiding this comment.
Can it be replaced with dynamic type?
https://reactjs.org/docs/jsx-in-depth.html#choosing-the-type-at-runtime
There was a problem hiding this comment.
Yes. I just choose render props over dynamic type due to the reasons:
- We're unsure whether the extracted types will be reused again, so just keep it as simple as possible.
- Render function don't create an extra layer of react components in the render tree
src/components/ImagePreviewer.js
Outdated
| return this.props.render({ | ||
| ...this.props, | ||
| request: undefined, | ||
| ...this.state.value |
There was a problem hiding this comment.
I feel like it should explicit say which prop to pass rather than this ... hack and remove prop using request: undefined.
There was a problem hiding this comment.
The ... operator is used widely and it's just the equivalent of Object.assign. Again, it keeps flexibility of defining react components/render functions here
|
@robertabcd anyway, I've made some changes according to your comments. Let me know if that looks good. |
| height: img.height | ||
| }); | ||
| img.onerror = reject; | ||
| img.src = src; |
There was a problem hiding this comment.
I see. Call to that function was removed in 8618472, so it currently only loads whitelisted sites. It might be good to resurrect that back later. It doesn't have to be this PR.
Depends on
#20Description
Rows to acomponents/Screencomponents/ImagePreviewerwithrenderprops patterncomponents/Screenterm_viewScreenshots