Language select updates #86
Conversation
| } | ||
|
|
||
| getPathname = () => { | ||
| let pathArray = this.props.location.pathname.split('/').filter(n => n) | ||
| let pathArray = this.props.location.pathname.split('/').filter(n => n); |
# Conflicts: # src/js/components/LanguageWrapper.js # src/js/modules/ListLink.js # src/js/page_sections/Header.js # src/js/page_sections/Navmenu.js # src/js/page_sections/RelatedLinks.js # src/js/pages/Service.js
| @@ -0,0 +1,16 @@ | |||
| import Cookies from 'js-cookie'; | |||
|
|
|||
| const getPathWithLangCode = (linkTo, lang) => { | |||
There was a problem hiding this comment.
@ifsimicoded, I merged this without your review so I wanted to call this out for your attention. Instead of finding a solution at the routing level, I chose to make this helper function, that can take a path and an optional lang parameters.
It looks up the cookie lang settings is there is no lang passed and returns the full path with language code.
This might be tedious to remember to use, but it felt like the quickest safest thing to do while there is still uncertainty about how our routing get moved into an SSR world.
| @@ -37,3 +37,9 @@ | |||
| content: "|"; | |||
| } | |||
| } | |||
|
|
|||
| .link { | |||
There was a problem hiding this comment.
minor, can we namespace our helpers with coa-
| @@ -11,7 +12,7 @@ class ListLink extends Component { | |||
| <Link | |||
| className={ `coa-ListLink ${(isBoxType && "coa-ListLink--boxprimary" )}` } | |||
| key={id} | |||
| to={url} | |||
| to={getPathWithLangCode(url)} | |||
There was a problem hiding this comment.
I like that you're trying to cover all app links to include language codes when present, but I think trying to manually build links with a helper this way is really fragile. We may forget to use this helper method/it requires a dev to KNOW to use a helper. Some links may come from Joplin's WYSIWYG editors or third party content. Maybe we can build a component wrapper for router Link components that do the language modification. And then devs only have to remember to use Link components for Links and this change is made in 1 place rather than many.
There was a problem hiding this comment.
Yup yup. Agreed on fragile and easy to forget.
For Joplin/3rd party content, I'm not sure that we should intervene and change links in that content. That seems a little unwieldy in a different way. But you're right, it should be a consideration. That type of parsing may also be a responsible we could give to the server. That would do the job more holistically for a world where the authored content goes to many clients, not just Janis.
As far as your suggestion around building a component wrapper for the Link... Are you saying, we write a new component called <Link> that itself imports the react-router-dom version of <Link> and we just move this getPathWithLangCode function into that component? That seems like a solid plan to me. Then the onus on the developer is to remember to import Link from the module we've created instead of from the standard library. Should we call it Link? Or LanguageLink? what do you think?
Updates UX of Language Select Bar. Closes #35