Allowing 'as' prop in Link and fixing 'href' prop #45
Conversation
|
Hi @capellini, thanks for a great contribution. However, although the matter of testing is something we desperately need (I asked for help awhile ago in #18), I'd like if you could separate these two issues into two distinct PRs:
Does that sound alright? It will make it easier to review, but more important make it easier to see where/how our testing setup began, without being mixed into actual component logic. |
Yes, of course. I should have thought to do that myself. I'll take out the testing stuff from this PR and add a separate PR for that. |
|
Okay thanks, that's great. Can you just tag me here again when you're comfortable with this PR being reviewed? |
|
@isaachinman -- ready for review |
src/components/Link.js
Outdated
| children: PropTypes.node.isRequired, | ||
| href: PropTypes.string.isRequired, | ||
| } | ||
|
|
||
| Link.defaultProps = { as: null } |
There was a problem hiding this comment.
Please add line breaks and space this as a normal object.
There was a problem hiding this comment.
How about adding Prettier to the project to avoid such issues in future? I'm using it everywhere and it saves a lot of times, especially in big teams. I can help with setting that up if there is interest.
There was a problem hiding this comment.
I'll add it in the future as we have more contributors. Not a huge priority right now as eslint will catch the important things.
There was a problem hiding this comment.
I originally had it that way, but CodeClimate failed because the file would have been over 25 lines long. Any chance that we can increase that limit on CodeClimate?
There was a problem hiding this comment.
We're not going to make decisions like this based on CodeClimate. Don't worry if such a thing causes it to "fail".
| ) | ||
| } | ||
| return <NextLink href={href}>{children}</NextLink> | ||
|
|
||
| return <NextLink href={href} as={as}>{children}</NextLink> |
There was a problem hiding this comment.
Is it safe to pass null into as?
There was a problem hiding this comment.
I had left off the defaultProps entirely, which would have left the 'as' prop as undefined, but that triggered the react/require-default-props linting rule. Perhaps I can disable that rule for this file, as I think it's valid to leave 'as' without a default prop. What do you think?
There was a problem hiding this comment.
It was a genuine question. If it's alright to pass null into as, I have no problems here. I suspect it is.
Otherwise, just put undefined as the default prop. I don't want to disable the react/require-default-props rule - it's important.
src/components/Link.js
Outdated
| let lng = null | ||
| if (Array.isArray(i18n.languages) && i18n.languages.length > 0) { | ||
| [lng] = i18n.languages | ||
| } | ||
| if (localeSubpaths && lng && lng !== defaultLanguage) { | ||
| const hrefWithLang = href.includes('?') ? `${href}&lng=${lng}` : `${href}?lng=${lng}` |
There was a problem hiding this comment.
Not sure if this is the safest way to do query-checking. There would be no harm in pulling in a third-party lib to do query mutations - I would actually prefer that.
Regardless, can you please use if/else statements instead of a ternary? It's especially confusing in this case, due to the '?'.
There was a problem hiding this comment.
👍
The if/else may also push us over our CodeClimate lines of code limit. Let me see what I can do.
|
@isaachinman - requested changes made...I "tested" the new |
Previously, if a user already passed in an 'as' prop to the i18n Link component, it would overwrite the 'as' prop to the next/link Link component. This checks to see if an 'as' prop has been passed in to the i18n link component and, if so, modifies it, rather than overwriting it. Also, if the 'href' prop happened to already have query parameters, the 'lng' parameter would be incorrectly added (e.g. if 'href=/foo?bar', then it would be modified to 'href=/foo?bar?lng=en'). This commit also checks to see if query parameters already exist and if, so, appends the lng parameter properly. Lastly, this commit adds some initial test infrastructure using enzyme and jest with code coverage and adds tests for the Link component.
capellini
force-pushed
the
link-as-prop
branch
from
1926b20 to
3189c47
Compare
|
@capellini Perhaps play around with the I've never used the lib you brought in, only query-string. I've updated the CodeClimate settings - do you mind doing a force push to trigger a build? |
capellini
force-pushed
the
link-as-prop
branch
from
3189c47 to
6417fa5
Compare
I played around a bit and it seems to work fine.
LMK if you would prefer that I use query-string and I can change the code...no problem. |
Previously, if a user already passed in an 'as' prop to the i18n Link
component, it would overwrite the 'as' prop to the next/link Link component.
This checks to see if an 'as' prop has been passed in to the i18n link
component and, if so, modifies it, rather than overwriting it.
Also, if the 'href' prop happened to already have query parameters, the 'lng'
parameter would be incorrectly added (e.g. if 'href=/foo?bar', then it would
be modified to 'href=/foo?bar?lng=en'). This commit also checks to see if
query parameters already exist and if, so, appends the lng parameter properly.
Lastly, this commit adds some initial test infrastructure using enzyme and
jest with code coverage and adds tests for the Link component.