migrate to react-router v6 #384
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: internetti The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| import 'bootstrap/dist/css/bootstrap.css'; | ||
| import 'bootstrap-icons/font/bootstrap-icons.css'; | ||
| import 'bootstrap/dist/js/bootstrap.bundle.min'; |
| <Link to="/clients/add" className="btn btn-sm btn-success"> | ||
| <Link to="add" className="btn btn-sm btn-success"> |
There was a problem hiding this comment.
Why has this changed but /clients/${c.id} hasn't?
There was a problem hiding this comment.
As in /clients/${c.id} should be ${c.id}?
There was a problem hiding this comment.
either way, they should be consistent, yes
| <NavLink className="nav-link" activeClassName="active" to="/organizations"> | ||
| <NavLink className="nav-link" to="/organizations"> |
There was a problem hiding this comment.
Are we losing some functionality here? I can't actually see NavLink in the v6 docs
There was a problem hiding this comment.
Found it.
<NavLink
to="tasks"
className={({ isActive }) =>
isActive ? activeClassName : undefined
}
>
There was a problem hiding this comment.
according to the docs, an active class will be applied by default when active, and since this is the class we've been using, I didn't see the need to add it ourselves.
| <Link className="btn btn-darkula btn-sm" to="organizations/add"> | ||
| <Link className="btn btn-darkula btn-sm" to="add"> |
There was a problem hiding this comment.
Same point here. Has something changed with the link behaviour? If it's a choice between add and /oragnizations/add the later is clearer.
There was a problem hiding this comment.
I thought just adding add to the current url was a more elegant solution, and actually more clear to me, but I won't insist on this.
There was a problem hiding this comment.
I'm in two minds about it. The full url is more readable, if I just look at "add" I don't really know what page I'm linking too. But if I were to change the url path I have less places to update it in if I just use "add".
Personally I'd go with the full url, but I don't have strong feelings when it comes to the pwa and authnz
| @@ -28,6 +28,7 @@ const AuthWrapper: React.FC<AuthWrapperProps> = ({ | |||
| onTokenChange, | |||
| injectToken = 'access_token', | |||
| }) => { | |||
| console.log('ATUH WRAPPER', scopes, clientId); | |||
internetti
force-pushed
the
storybookDocsLibUpdate
branch
from
3df7b1e to
b0e9160
Compare
|
@internetti: Adding label Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
unify versions of native base
internetti
force-pushed
the
storybookDocsLibUpdate
branch
from
b0e9160 to
64b67b7
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
| "@storybook/addon-ondevice-knobs": "~5.3.25", | ||
| "@storybook/react-native": "~5.3.25", | ||
| "@storybook/react-native-server": "~5.3.23", | ||
| "@react-native-async-storage/async-storage": "^1.16.1", |
There was a problem hiding this comment.
Why this package? Is it a peer dep?
There was a problem hiding this comment.
yes, of @storybook/react-native
|
/lgtm |
the documentation feature of storybook requires an upgrade to version 6.
this version updated their dependency to react router to v6, which had a lot of breaking changes.
stack overflow thread about that
this pull request migrates the pwa and the authnz frontend to the new react router version