You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/07/27 15:08:35 UTC
[GitHub] [superset] kgabryje opened a new pull request, #20890: feat: Use SPA navigation from datasets list to Explore
kgabryje opened a new pull request, #20890:
URL: https://github.com/apache/superset/pull/20890
### SUMMARY
This PR implements SPA navigation in Datasets list. By default, clicking on dataset's name opens Explore, but user can override it with some external URL. Default react-router Link component can't handle external URLs, so I implemented a `GenericLink` component, which check if URL starts with `https://` - if it does, use HTML anchor element, else use react-router Link.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #20890: feat: Use SPA navigation from datasets list to Explore
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20890:
URL: https://github.com/apache/superset/pull/20890#discussion_r932539522
##########
superset-frontend/src/components/GenericLink/GenericLink.tsx:
##########
@@ -0,0 +1,51 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { Link, LinkProps } from 'react-router-dom';
+
+export const GenericLink = ({
+ to,
+ component,
+ replace,
+ innerRef,
+ children,
+ ...rest
+}: // css prop type check was failing, override with any
+LinkProps & { css?: any }) => {
+ if (typeof to === 'string' && /^https?:\/\//.test(to)) {
Review Comment:
> This PR, even with more foolproof external url detection, wouldn't change that behaviour - we'd still get an <a> element linking to http://localhost:9000/tablemodelview/list/www.google.com.
I was thinking that upon detecting www.google.com as an external link without a protocol, then the component would add the protocol automatically, setting the `<a>` tag to `http://www.google.com`. Of course, we can implement this outside of the component but in that case we would need to always call the helper function before using the component.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #20890: feat: Use SPA navigation from datasets list to Explore
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20890:
URL: https://github.com/apache/superset/pull/20890#discussion_r932350857
##########
superset-frontend/src/components/GenericLink/GenericLink.tsx:
##########
@@ -0,0 +1,51 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { Link, LinkProps } from 'react-router-dom';
+
+export const GenericLink = ({
+ to,
+ component,
+ replace,
+ innerRef,
+ children,
+ ...rest
+}: // css prop type check was failing, override with any
+LinkProps & { css?: any }) => {
+ if (typeof to === 'string' && /^https?:\/\//.test(to)) {
Review Comment:
You can design the component with the protocol restriction, but then we need to handle the problem elsewhere because it will be really common to have users type the URL without the protocol. In this case, when they click on the dataset nothing happens.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #20890: feat: Use SPA navigation from datasets list to Explore
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #20890:
URL: https://github.com/apache/superset/pull/20890#discussion_r932293318
##########
superset-frontend/src/components/GenericLink/GenericLink.tsx:
##########
@@ -0,0 +1,51 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { Link, LinkProps } from 'react-router-dom';
+
+export const GenericLink = ({
+ to,
+ component,
+ replace,
+ innerRef,
+ children,
+ ...rest
+}: // css prop type check was failing, override with any
+LinkProps & { css?: any }) => {
+ if (typeof to === 'string' && /^https?:\/\//.test(to)) {
Review Comment:
Actually that's not really a valid concern in this case. URL passed to `<a>` must have a protocol prefix, otherwise it's appended to current pathname. In such case it doesn't matter if we use `<a>` or `<Link>`.
<img width="326" alt="image" src="https://user-images.githubusercontent.com/15073128/181546680-1f0a4e4c-0564-4ebc-bcd7-5ef12a5934b5.png">
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] EugeneTorap commented on pull request #20890: feat: Use SPA navigation from datasets list to Explore
Posted by GitBox <gi...@apache.org>.
EugeneTorap commented on PR #20890:
URL: https://github.com/apache/superset/pull/20890#issuecomment-1202076095
Hi @kgabryje. Dataset links don't work in SPA in **_Dataset_** column on **_Charts_** page
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #20890: feat: Use SPA navigation from datasets list to Explore
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #20890:
URL: https://github.com/apache/superset/pull/20890#discussion_r932509758
##########
superset-frontend/src/components/GenericLink/GenericLink.tsx:
##########
@@ -0,0 +1,51 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { Link, LinkProps } from 'react-router-dom';
+
+export const GenericLink = ({
+ to,
+ component,
+ replace,
+ innerRef,
+ children,
+ ...rest
+}: // css prop type check was failing, override with any
+LinkProps & { css?: any }) => {
+ if (typeof to === 'string' && /^https?:\/\//.test(to)) {
Review Comment:
Yeah I see your point. However, I'm wondering if that should be considered a separate bug. Currently, if you type "www.google.com" in default url field, the link would redirect you to `http://localhost:9000/tablemodelview/list/www.google.com`.
This PR, even with more foolproof external url detection, wouldn't change that behaviour - we'd still get an `<a>` element linking to http://localhost:9000/tablemodelview/list/www.google.com.
I'd propose adding external URL detection in this PR (even though it wouldn't really do anything) and handling external links without protocol in a separate PR
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #20890: feat: Use SPA navigation from datasets list to Explore
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #20890:
URL: https://github.com/apache/superset/pull/20890#discussion_r932247968
##########
superset-frontend/src/components/GenericLink/GenericLink.tsx:
##########
@@ -0,0 +1,51 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { Link, LinkProps } from 'react-router-dom';
+
+export const GenericLink = ({
+ to,
+ component,
+ replace,
+ innerRef,
+ children,
+ ...rest
+}: // css prop type check was failing, override with any
+LinkProps & { css?: any }) => {
+ if (typeof to === 'string' && /^https?:\/\//.test(to)) {
Review Comment:
True. Found some more complex regex, I'll use that one instead + some unit tests
##########
superset-frontend/src/components/GenericLink/GenericLink.tsx:
##########
@@ -0,0 +1,51 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { Link, LinkProps } from 'react-router-dom';
+
+export const GenericLink = ({
+ to,
+ component,
+ replace,
+ innerRef,
+ children,
+ ...rest
+}: // css prop type check was failing, override with any
Review Comment:
Oh nice!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] EugeneTorap commented on pull request #20890: feat: Use SPA navigation from datasets list to Explore
Posted by GitBox <gi...@apache.org>.
EugeneTorap commented on PR #20890:
URL: https://github.com/apache/superset/pull/20890#issuecomment-1202092115
Also in **_Dashboard_**
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #20890: feat: Use SPA navigation from datasets list to Explore
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20890:
URL: https://github.com/apache/superset/pull/20890#discussion_r933522732
##########
superset-frontend/src/components/GenericLink/GenericLink.tsx:
##########
@@ -29,9 +30,9 @@ export const GenericLink = <S,>({
...rest
}: React.PropsWithoutRef<LinkProps<S>> &
React.RefAttributes<HTMLAnchorElement>) => {
- if (typeof to === 'string' && /^https?:\/\//.test(to)) {
+ if (typeof to === 'string' && isUrlExternal(to)) {
return (
- <a data-test="external-link" href={to} {...rest}>
+ <a data-test="external-link" href={parseUrl(to)} {...rest}>
Review Comment:
Small suggestion for a follow-up: You could return the parsed value when it's valid and undefined or another thing when it's invalid. That way you would only need to apply the regex once by combining `isUrlExternal` with `parseUrl`.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #20890: feat: Use SPA navigation from datasets list to Explore
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20890:
URL: https://github.com/apache/superset/pull/20890#discussion_r932220249
##########
superset-frontend/src/components/GenericLink/GenericLink.tsx:
##########
@@ -0,0 +1,51 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { Link, LinkProps } from 'react-router-dom';
+
+export const GenericLink = ({
+ to,
+ component,
+ replace,
+ innerRef,
+ children,
+ ...rest
+}: // css prop type check was failing, override with any
Review Comment:
The type of `Link` properties is not `LinkProps` but `React.PropsWithoutRef<LinkProps<S>> & React.RefAttributes<HTMLAnchorElement>`.
To fix the Typescript error, you can define your component like:
```
export const GenericLink = <S,>({
to,
component,
replace,
innerRef,
children,
...rest
}: React.PropsWithoutRef<LinkProps<S>> &
React.RefAttributes<HTMLAnchorElement>) => {
...
}
```
##########
superset-frontend/src/components/GenericLink/GenericLink.tsx:
##########
@@ -0,0 +1,51 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { Link, LinkProps } from 'react-router-dom';
+
+export const GenericLink = ({
+ to,
+ component,
+ replace,
+ innerRef,
+ children,
+ ...rest
+}: // css prop type check was failing, override with any
+LinkProps & { css?: any }) => {
+ if (typeof to === 'string' && /^https?:\/\//.test(to)) {
Review Comment:
I think we need to improve the algorithm to detect internal and external URLs. If we type `www.google.com` without the `https` then it breaks. I'm not sure if we already have something similar in the codebase or if we need to find an npm package for this.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #20890: feat: Use SPA navigation from datasets list to Explore
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #20890:
URL: https://github.com/apache/superset/pull/20890#issuecomment-1196980369
# [Codecov](https://codecov.io/gh/apache/superset/pull/20890?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#20890](https://codecov.io/gh/apache/superset/pull/20890?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (cbabb4f) into [master](https://codecov.io/gh/apache/superset/commit/77db0651d819f4bda367fc59a4e95954cb0929e1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (77db065) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #20890 +/- ##
=======================================
Coverage 66.25% 66.25%
=======================================
Files 1758 1759 +1
Lines 67048 67052 +4
Branches 7118 7119 +1
=======================================
+ Hits 44423 44427 +4
Misses 20808 20808
Partials 1817 1817
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `51.91% <100.00%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/20890?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...rontend/src/components/GenericLink/GenericLink.tsx](https://codecov.io/gh/apache/superset/pull/20890/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvR2VuZXJpY0xpbmsvR2VuZXJpY0xpbmsudHN4) | `100.00% <100.00%> (ø)` | |
| [...ontend/src/views/CRUD/data/dataset/DatasetList.tsx](https://codecov.io/gh/apache/superset/pull/20890/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhc2V0L0RhdGFzZXRMaXN0LnRzeA==) | `56.20% <100.00%> (ø)` | |
Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #20890: feat: Use SPA navigation from datasets list to Explore
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #20890:
URL: https://github.com/apache/superset/pull/20890#discussion_r934614017
##########
superset-frontend/src/components/GenericLink/GenericLink.tsx:
##########
@@ -29,9 +30,9 @@ export const GenericLink = <S,>({
...rest
}: React.PropsWithoutRef<LinkProps<S>> &
React.RefAttributes<HTMLAnchorElement>) => {
- if (typeof to === 'string' && /^https?:\/\//.test(to)) {
+ if (typeof to === 'string' && isUrlExternal(to)) {
return (
- <a data-test="external-link" href={to} {...rest}>
+ <a data-test="external-link" href={parseUrl(to)} {...rest}>
Review Comment:
Good point! I'll do that in a separate PR
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #20890: feat: Use SPA navigation from datasets list to Explore
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #20890:
URL: https://github.com/apache/superset/pull/20890#discussion_r933187824
##########
superset-frontend/src/components/GenericLink/GenericLink.tsx:
##########
@@ -0,0 +1,51 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { Link, LinkProps } from 'react-router-dom';
+
+export const GenericLink = ({
+ to,
+ component,
+ replace,
+ innerRef,
+ children,
+ ...rest
+}: // css prop type check was failing, override with any
+LinkProps & { css?: any }) => {
+ if (typeof to === 'string' && /^https?:\/\//.test(to)) {
Review Comment:
Done :)
https://user-images.githubusercontent.com/15073128/181757636-957f38fb-9640-4c9f-a21e-19dc2763cd8a.mov
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje merged pull request #20890: feat: Use SPA navigation from datasets list to Explore
Posted by GitBox <gi...@apache.org>.
kgabryje merged PR #20890:
URL: https://github.com/apache/superset/pull/20890
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] EugeneTorap commented on pull request #20890: feat: Use SPA navigation from datasets list to Explore
Posted by GitBox <gi...@apache.org>.
EugeneTorap commented on PR #20890:
URL: https://github.com/apache/superset/pull/20890#issuecomment-1202092587
@michael-s-molina @kgabryje I will create a new PR for this fix
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org