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/01/20 08:22:09 UTC
[GitHub] [superset] ad-m opened a new pull request #18105: refactor: migrate RowCountLabel to TypeScript & added story
ad-m opened a new pull request #18105:
URL: https://github.com/apache/superset/pull/18105
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Non-fuctional change to migrate `superset-frontend/src/explore/components/RowCountLabel.{jsx,tsx}`.
In addiction story for `RowCountLabel` has been added. To use that storybook have been enabled for `explore` directory.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No visual changes:
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
Use Storybook to see no visual & functional changes. You may also use build frontend.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue: see #18100
- [x] Required feature flags: no
- [x] Changes UI: no
- [x] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)): no
- [x] Migration is atomic, supports rollback & is backwards-compatible: N/A
- [x] Confirm DB migration upgrade and downgrade tested: N/A
- [x] Runtime estimates and downtime expectations provided: N/A
- [x] Introduces new feature or API: no
- [x] Removes existing feature or API: no
@kgabryje, cześć, czy byłbyś tak miły, aby na to spojrzeć? 😉
--
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 change in pull request #18105: refactor: migrate RowCountLabel to TypeScript & added story
Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #18105:
URL: https://github.com/apache/superset/pull/18105#discussion_r788646656
##########
File path: superset-frontend/src/explore/components/RowCountLabel.tsx
##########
@@ -31,11 +31,20 @@ const propTypes = {
loading: PropTypes.bool,
};
+type RowCountLabelProps = {
+ rowcount?: number;
+ limit?: number;
+ rows?: string;
Review comment:
I don't think we use prop `rows` here 🤔
--
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] ad-m commented on a change in pull request #18105: refactor: migrate RowCountLabel to TypeScript & added story
Posted by GitBox <gi...@apache.org>.
ad-m commented on a change in pull request #18105:
URL: https://github.com/apache/superset/pull/18105#discussion_r788689627
##########
File path: superset-frontend/src/explore/components/RowCountLabel.tsx
##########
@@ -31,11 +31,20 @@ const propTypes = {
loading: PropTypes.bool,
};
+type RowCountLabelProps = {
+ rowcount?: number;
+ limit?: number;
+ rows?: string;
+ suffix?: string;
+ loading?: boolean;
+};
+
const defaultProps = {
Review comment:
You're right, when removing `defaultProps` while removing `propTypes` I changed it this way right away, according to the current convention.
--
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 change in pull request #18105: refactor: migrate RowCountLabel to TypeScript & added story
Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #18105:
URL: https://github.com/apache/superset/pull/18105#discussion_r788642494
##########
File path: superset-frontend/src/explore/components/RowCountLabel.tsx
##########
@@ -31,11 +31,20 @@ const propTypes = {
loading: PropTypes.bool,
};
+type RowCountLabelProps = {
+ rowcount?: number;
Review comment:
I think we should make the rowcount required
--
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 #18105: refactor: migrate RowCountLabel to TypeScript & added story
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #18105:
URL: https://github.com/apache/superset/pull/18105#issuecomment-1017328830
# [Codecov](https://codecov.io/gh/apache/superset/pull/18105?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 [#18105](https://codecov.io/gh/apache/superset/pull/18105?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f8cb8d7) into [master](https://codecov.io/gh/apache/superset/commit/035638c95869d0d4b7c9d44f13e88e3535ecdf4c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (035638c) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/18105?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #18105 +/- ##
=======================================
Coverage 65.86% 65.86%
=======================================
Files 1577 1577
Lines 61809 61810 +1
Branches 6242 6242
=======================================
+ Hits 40708 40709 +1
Misses 19501 19501
Partials 1600 1600
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `50.92% <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/18105?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...-frontend/src/explore/components/RowCountLabel.tsx](https://codecov.io/gh/apache/superset/pull/18105/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9Sb3dDb3VudExhYmVsLnRzeA==) | `100.00% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/18105?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/18105?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [035638c...f8cb8d7](https://codecov.io/gh/apache/superset/pull/18105?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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 merged pull request #18105: refactor: migrate RowCountLabel to TypeScript & added story
Posted by GitBox <gi...@apache.org>.
kgabryje merged pull request #18105:
URL: https://github.com/apache/superset/pull/18105
--
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] ad-m commented on a change in pull request #18105: refactor: migrate RowCountLabel to TypeScript & added story
Posted by GitBox <gi...@apache.org>.
ad-m commented on a change in pull request #18105:
URL: https://github.com/apache/superset/pull/18105#discussion_r788688540
##########
File path: superset-frontend/src/explore/components/RowCountLabel.tsx
##########
@@ -31,11 +31,20 @@ const propTypes = {
loading: PropTypes.bool,
};
+type RowCountLabelProps = {
+ rowcount?: number;
+ limit?: number;
+ rows?: string;
Review comment:
You're right. I tried not to change more widely (this field was previously in propTypes) to facilitate the review, but - taking into account your comments - I will introduce such optimizations in future migrations as well.
Thanks for your comments. They are very valuable to me. At the beginning, I just wanted to touch small things to calibrate myself to the style, the expected unit of work in the project, so that our cooperation would go smoothly. There are a lot of components to migrate and I don't want to get stuck in a big PR review.
--
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 change in pull request #18105: refactor: migrate RowCountLabel to TypeScript & added story
Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #18105:
URL: https://github.com/apache/superset/pull/18105#discussion_r788666561
##########
File path: superset-frontend/src/explore/components/RowCountLabel.tsx
##########
@@ -31,11 +31,20 @@ const propTypes = {
loading: PropTypes.bool,
};
Review comment:
Well spotted! I'm quite sure that's a mistake
--
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 change in pull request #18105: refactor: migrate RowCountLabel to TypeScript & added story
Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #18105:
URL: https://github.com/apache/superset/pull/18105#discussion_r788640576
##########
File path: superset-frontend/src/explore/components/RowCountLabel.tsx
##########
@@ -31,11 +31,20 @@ const propTypes = {
loading: PropTypes.bool,
};
Review comment:
Now that we use typescript we don't need propTypes anymore 🙂
--
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] edited a comment on pull request #18105: refactor: migrate RowCountLabel to TypeScript & added story
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #18105:
URL: https://github.com/apache/superset/pull/18105#issuecomment-1017328830
# [Codecov](https://codecov.io/gh/apache/superset/pull/18105?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 [#18105](https://codecov.io/gh/apache/superset/pull/18105?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f8cb8d7) into [master](https://codecov.io/gh/apache/superset/commit/035638c95869d0d4b7c9d44f13e88e3535ecdf4c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (035638c) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.
> :exclamation: Current head f8cb8d7 differs from pull request most recent head ddedb2e. Consider uploading reports for the commit ddedb2e to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/18105?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #18105 +/- ##
=======================================
Coverage 65.86% 65.86%
=======================================
Files 1577 1577
Lines 61809 61810 +1
Branches 6242 6242
=======================================
+ Hits 40708 40709 +1
Misses 19501 19501
Partials 1600 1600
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `50.92% <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/18105?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...-frontend/src/explore/components/RowCountLabel.tsx](https://codecov.io/gh/apache/superset/pull/18105/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9Sb3dDb3VudExhYmVsLnRzeA==) | `100.00% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/18105?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/18105?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [035638c...ddedb2e](https://codecov.io/gh/apache/superset/pull/18105?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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 change in pull request #18105: refactor: migrate RowCountLabel to TypeScript & added story
Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #18105:
URL: https://github.com/apache/superset/pull/18105#discussion_r788649849
##########
File path: superset-frontend/src/explore/components/RowCountLabel.tsx
##########
@@ -31,11 +31,20 @@ const propTypes = {
loading: PropTypes.bool,
};
+type RowCountLabelProps = {
+ rowcount?: number;
+ limit?: number;
+ rows?: string;
+ suffix?: string;
+ loading?: boolean;
+};
+
const defaultProps = {
Review comment:
To make sure that `defaultProps` types follow are compatible with `RowCountLabelProps`, we should either add `& typeof defaultProps` to type definition or... get rid of defaultProps and just do `const { rowcount, limit, suffix = t('rows'), loading } = props;`.
According to https://react-typescript-cheatsheet.netlify.app/docs/basic/getting-started/default_props/, the second solution is preferred
##########
File path: superset-frontend/src/explore/components/RowCountLabel.tsx
##########
@@ -31,11 +31,20 @@ const propTypes = {
loading: PropTypes.bool,
};
+type RowCountLabelProps = {
+ rowcount?: number;
+ limit?: number;
+ rows?: string;
+ suffix?: string;
+ loading?: boolean;
+};
+
const defaultProps = {
Review comment:
To make sure that `defaultProps` types are compatible with `RowCountLabelProps`, we should either add `& typeof defaultProps` to type definition or... get rid of defaultProps and just do `const { rowcount, limit, suffix = t('rows'), loading } = props;`.
According to https://react-typescript-cheatsheet.netlify.app/docs/basic/getting-started/default_props/, the second solution is preferred
--
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 pull request #18105: refactor: migrate RowCountLabel to TypeScript & added story
Posted by GitBox <gi...@apache.org>.
kgabryje commented on pull request #18105:
URL: https://github.com/apache/superset/pull/18105#issuecomment-1017371720
Thank you for the contribution, such work is always soo appreciated 🙂 I left a few small comments, but other than that the PR looks great!
--
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 change in pull request #18105: refactor: migrate RowCountLabel to TypeScript & added story
Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #18105:
URL: https://github.com/apache/superset/pull/18105#discussion_r788663985
##########
File path: superset-frontend/src/explore/components/RowCountLabel.stories.tsx
##########
@@ -0,0 +1,59 @@
+/**
+ * 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 RowCountLabel from './RowCountLabel';
+
+export default {
+ title: 'RowCountLabel',
+ component: RowCountLabel,
+};
+
+const options = {
+ loading: {
+ loading: true,
Review comment:
I think we need to either add `rowcount: 0` here or set `rowcount = 0` as a default value in the component. Now we get a red "danger" colour in storybook, which shouldn't happen.
--
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 change in pull request #18105: refactor: migrate RowCountLabel to TypeScript & added story
Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #18105:
URL: https://github.com/apache/superset/pull/18105#discussion_r788666561
##########
File path: superset-frontend/src/explore/components/RowCountLabel.tsx
##########
@@ -31,11 +31,20 @@ const propTypes = {
loading: PropTypes.bool,
};
Review comment:
Well spotted! I'm quite sure that's a mistake. @suddjian I think that's your piece of code, could you please confirm?
--
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 change in pull request #18105: refactor: migrate RowCountLabel to TypeScript & added story
Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #18105:
URL: https://github.com/apache/superset/pull/18105#discussion_r788649849
##########
File path: superset-frontend/src/explore/components/RowCountLabel.tsx
##########
@@ -31,11 +31,20 @@ const propTypes = {
loading: PropTypes.bool,
};
+type RowCountLabelProps = {
+ rowcount?: number;
+ limit?: number;
+ rows?: string;
+ suffix?: string;
+ loading?: boolean;
+};
+
const defaultProps = {
Review comment:
To make sure that `defaultProps` types are compatible with `RowCountLabelProps`, we should either add `& typeof defaultProps` to type definition or... get rid of defaultProps and just use ES6 default values like `const { rowcount, limit, suffix = t('rows'), loading } = props;`.
According to https://react-typescript-cheatsheet.netlify.app/docs/basic/getting-started/default_props/, the second solution is preferred
--
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] ad-m commented on a change in pull request #18105: refactor: migrate RowCountLabel to TypeScript & added story
Posted by GitBox <gi...@apache.org>.
ad-m commented on a change in pull request #18105:
URL: https://github.com/apache/superset/pull/18105#discussion_r788660586
##########
File path: superset-frontend/src/explore/components/RowCountLabel.tsx
##########
@@ -31,11 +31,20 @@ const propTypes = {
loading: PropTypes.bool,
};
Review comment:
Thanks for the clarification. I am not fully aware of project standards whether `propTypes` is not used in an unusual way, and [have found TypeScript components in Superset](https://github.com/apache/superset/blob/master/superset-frontend/src/explore/components/controls/VizTypeControl/index.tsx) to retain `propTypes` sometime.
--
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] edited a comment on pull request #18105: refactor: migrate RowCountLabel to TypeScript & added story
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #18105:
URL: https://github.com/apache/superset/pull/18105#issuecomment-1017328830
# [Codecov](https://codecov.io/gh/apache/superset/pull/18105?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 [#18105](https://codecov.io/gh/apache/superset/pull/18105?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ddedb2e) into [master](https://codecov.io/gh/apache/superset/commit/035638c95869d0d4b7c9d44f13e88e3535ecdf4c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (035638c) will **decrease** coverage by `0.00%`.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/superset/pull/18105?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #18105 +/- ##
==========================================
- Coverage 65.86% 65.85% -0.01%
==========================================
Files 1577 1577
Lines 61809 61806 -3
Branches 6242 6243 +1
==========================================
- Hits 40708 40704 -4
Misses 19501 19501
- Partials 1600 1601 +1
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `50.91% <0.00%> (-0.01%)` | :arrow_down: |
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/18105?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...-frontend/src/explore/components/RowCountLabel.tsx](https://codecov.io/gh/apache/superset/pull/18105/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9Sb3dDb3VudExhYmVsLnRzeA==) | `88.88% <0.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/18105?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/18105?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [035638c...ddedb2e](https://codecov.io/gh/apache/superset/pull/18105?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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