e2corporation commented on code in PR #3327:
URL: https://github.com/apache/incubator-devlake/pull/3327#discussion_r991444027
##########
config-ui/src/components/Sidebar/SidebarMenu.jsx:
##########
@@ -18,13 +18,25 @@
import React, { useEffect, Fragment } from 'react'
import { Menu } from '@blueprintjs/core'
import '@/styles/sidebar-menu.scss'
+import { useHistory } from 'react-router-dom'
const SidebarMenu = (props) => {
+ const history = useHistory()
const { menu } = props
// const activeRoute = useRouteMatch()
useEffect(() => {}, [menu])
+ const handleProviderClick = (route) => {
Review Comment:
@likyh While this navigation handler will solve the problem of force-reload
and correctly utilize push routing, by using a `window.location.href` call
directly, this will ignore the `target` attributes defined on the navigation
links, so external resources will no longer open in a new tab. (When our menu
has _external_ links we open them in a new-tab so the user never gets routed
away from Config-UI, see `target` attribute for each menu item configuration)
An alternate approach to consider is to also pass the Event Object `e` to
this handler, and use `stopPropagation` instead.
```suggestion
const handleProviderClick = (e, route) => {
if (route && !route.includes('//')) {
e.stopPropagation()
history.push(route)
}
// else {
// Do-nothing let Click Propagation Continue...
// }
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
