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/26 20:07:44 UTC
[GitHub] [superset] cccs-RyanK opened a new pull request, #20876: feature: Frontend tagging
cccs-RyanK opened a new pull request, #20876:
URL: https://github.com/apache/superset/pull/20876
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
This work was done by [@cccs-nik](https://github.com/cccs-nik) on our own fork of superset.
Currently, there exists tagging for dashboards, charts, and saved queries in the backend. This PR introduces Tagging to the front-end and allows users to view and edit tags in the UI. Feedback is greatly appreciated!
The following are some features introduced:
- Tags can be seen and are searchable in the list view for dashboards, charts and saved queries. Tags can be added via the "edit properties" modal for dashboards and charts, but there is currently no way to add tags to saved queries.
- Clicking on a tag will take the user to a new page that displays all objects with that tag.
- Tags are automatically applied to all objects that contain the type of an object and the owner of the object, but only "custom" type tags are displayed in the UI.
The future work for this will be to add tags for saved queries somewhere in the UI and to add tagging for datasets to both the frontend and the backend.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
Some examples for tagging dashboards
Dashboard List
Dashboard Header
Edit Properties for Dashboard
Tags View (endpoint: /superset/tags/?tags=[tag_name])
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
- Set feature flag TAGGING_SYSTEM to true in superset/config.py
- navigate to the edit properties modal for either charts or dashboards to add tags
- view tags from the lists or headers for dashboards or charts.
- click on tags or navigate to the 'Tags' tab to search objects by tags.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue:
- [x] Required feature flags:
- [x] 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
- [x] 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] github-actions[bot] commented on pull request #20876: feat: Frontend tagging
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #20876:
URL: https://github.com/apache/superset/pull/20876#issuecomment-1439125168
Ephemeral environment shutdown and build artifacts deleted.
--
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] yousoph commented on pull request #20876: feat: Frontend tagging
Posted by "yousoph (via GitHub)" <gi...@apache.org>.
yousoph commented on PR #20876:
URL: https://github.com/apache/superset/pull/20876#issuecomment-1410858396
/testenv up FEATURE_TAGGING_SYSTEM=true
--
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 #20876: feature: Frontend tagging
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #20876:
URL: https://github.com/apache/superset/pull/20876#issuecomment-1197261041
# [Codecov](https://codecov.io/gh/apache/superset/pull/20876?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 [#20876](https://codecov.io/gh/apache/superset/pull/20876?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1aba266) 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 **decrease** coverage by `11.55%`.
> The diff coverage is `70.04%`.
```diff
@@ Coverage Diff @@
## master #20876 +/- ##
===========================================
- Coverage 66.25% 54.69% -11.56%
===========================================
Files 1758 1763 +5
Lines 67048 67223 +175
Branches 7118 7118
===========================================
- Hits 44423 36769 -7654
- Misses 20808 28637 +7829
Partials 1817 1817
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `53.33% <71.35%> (+0.08%)` | :arrow_up: |
| mysql | `?` | |
| postgres | `?` | |
| presto | `?` | |
| python | `57.65% <71.35%> (-23.91%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `50.36% <71.35%> (+0.10%)` | :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/20876?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ackages/superset-ui-core/src/utils/featureFlags.ts](https://codecov.io/gh/apache/superset/pull/20876/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvdXRpbHMvZmVhdHVyZUZsYWdzLnRz) | `100.00% <ø> (ø)` | |
| [superset-frontend/src/views/routes.tsx](https://codecov.io/gh/apache/superset/pull/20876/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL3JvdXRlcy50c3g=) | `55.55% <ø> (ø)` | |
| [superset/charts/api.py](https://codecov.io/gh/apache/superset/pull/20876/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-c3VwZXJzZXQvY2hhcnRzL2FwaS5weQ==) | `50.37% <ø> (-35.61%)` | :arrow_down: |
| [superset/dashboards/api.py](https://codecov.io/gh/apache/superset/pull/20876/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-c3VwZXJzZXQvZGFzaGJvYXJkcy9hcGkucHk=) | `51.15% <ø> (-41.33%)` | :arrow_down: |
| [superset/tags/commands/create.py](https://codecov.io/gh/apache/superset/pull/20876/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-c3VwZXJzZXQvdGFncy9jb21tYW5kcy9jcmVhdGUucHk=) | `43.33% <43.33%> (ø)` | |
| [superset/tags/dao.py](https://codecov.io/gh/apache/superset/pull/20876/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-c3VwZXJzZXQvdGFncy9kYW8ucHk=) | `44.82% <44.82%> (ø)` | |
| [superset/models/tags.py](https://codecov.io/gh/apache/superset/pull/20876/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-c3VwZXJzZXQvbW9kZWxzL3RhZ3MucHk=) | `78.26% <50.00%> (-0.69%)` | :arrow_down: |
| [...src/dashboard/components/PropertiesModal/index.tsx](https://codecov.io/gh/apache/superset/pull/20876/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL1Byb3BlcnRpZXNNb2RhbC9pbmRleC50c3g=) | `62.19% <53.33%> (ø)` | |
| [...perset-frontend/src/views/CRUD/chart/ChartList.tsx](https://codecov.io/gh/apache/superset/pull/20876/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvY2hhcnQvQ2hhcnRMaXN0LnRzeA==) | `59.13% <62.50%> (ø)` | |
| [...d/src/explore/components/PropertiesModal/index.tsx](https://codecov.io/gh/apache/superset/pull/20876/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9Qcm9wZXJ0aWVzTW9kYWwvaW5kZXgudHN4) | `63.93% <63.15%> (ø)` | |
| ... and [317 more](https://codecov.io/gh/apache/superset/pull/20876/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
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] hughhhh commented on a diff in pull request #20876: feat: Frontend tagging
Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh commented on code in PR #20876:
URL: https://github.com/apache/superset/pull/20876#discussion_r1107667613
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -314,6 +421,26 @@ function PropertiesModal({
)}
</StyledHelpBlock>
</FormItem>
+ {isFeatureEnabled(FeatureFlag.TAGGING_SYSTEM) && (
+ <h3 style={{ marginTop: '1em' }}>{t('Tags')}</h3>
Review Comment:
can we address this concern? @cccs-RyanK
--
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] github-actions[bot] commented on pull request #20876: feat: Frontend tagging
Posted by github-actions.
github-actions[bot] commented on PR #20876:
URL: https://github.com/apache/superset/pull/20876#issuecomment-1410860873
@yousoph Ephemeral environment spinning up at http://35.91.84.185:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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] cccs-RyanK commented on pull request #20876: feat: Frontend tagging
Posted by "cccs-RyanK (via GitHub)" <gi...@apache.org>.
cccs-RyanK commented on PR #20876:
URL: https://github.com/apache/superset/pull/20876#issuecomment-1420673538
> overall looks good but left some comments
Added tests and moved the those endpoints under tags/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] yousoph commented on pull request #20876: feat: Frontend tagging
Posted by GitBox <gi...@apache.org>.
yousoph commented on PR #20876:
URL: https://github.com/apache/superset/pull/20876#issuecomment-1353349306
One bug I noticed:
`testTag` has already been selected, but it shows up in the list as unselected. I can select it from the list, see 2 of the same tag, and save, but once I refresh it goes back to this state.
--
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] eschutho merged pull request #20876: feat: Frontend tagging
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho merged PR #20876:
URL: https://github.com/apache/superset/pull/20876
--
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] hughhhh commented on a diff in pull request #20876: feat: Frontend tagging
Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh commented on code in PR #20876:
URL: https://github.com/apache/superset/pull/20876#discussion_r1086946230
##########
superset/tags/commands/create.py:
##########
@@ -0,0 +1,57 @@
+# 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 logging
+from typing import Any, Dict
+
+from flask_appbuilder.models.sqla import Model
+
+from superset.commands.base import BaseCommand, CreateMixin
+from superset.dao.exceptions import DAOCreateFailedError
+from superset.tags.commands.exceptions import TagCreateFailedError, TagInvalidError
+from superset.tags.dao import TagDAO
+from superset.tags.models import ObjectTypes
+
+logger = logging.getLogger(__name__)
+
+
+class CreateCustomTagCommand(CreateMixin, BaseCommand):
Review Comment:
can we write a command test 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] kasiazjc commented on pull request #20876: feature: Frontend tagging
Posted by GitBox <gi...@apache.org>.
kasiazjc commented on PR #20876:
URL: https://github.com/apache/superset/pull/20876#issuecomment-1198490801
> Hi all, thanks for the great work on this! I have a few UX/UI suggestions that would better align with Superset's current patterns.
>
> **Edit properties modal** Instead of putting the applied tags to the right of the tag selection field, I would use the multi-select component and keep the tags inside the bounds of the field (like the multi-select filter field in dashboards): <img alt="Screen Shot 2022-07-28 at 10 49 11 AM" width="250" src="https://user-images.githubusercontent.com/90715511/181603984-0c910a36-fbcc-4024-a976-59a57c953984.png">
>
> **Tags view** I would actually create two views here to properly utilize and manage tags.
>
> 1. Tags CRUD
> A Tags page, accessible from the global settings menu, that uses the existing CRUD template to display all tags. Columns in the table could include how many entities are tagged, who created it, etc. This is where the user would manage (edit/add/delete) tags.
> 2. "All entities" view
> To satisfy the need to see all entities with a certain tag applied, I would create another CRUD view that combines dashboards/charts/queries and allows the user to filter by tags. This could be accessible from the main navigation.
Thanks for working on this! Have a few additional comments.
**Crud view**
I think the field in the filter bar should be a multi select, not a search. Search does not show all of the possible tags and I think it would be hard to remember all of them to choose the right one.
Our multiple select currently expands in height when multiple values are selected before truncating, so it would have to have fixed height and truncate earlier.
**Object pages**
Do tags have to be displayed in the header? I guess they will be mostly used for searching/grouping things.
1. It clutters the header + the question on what is more important comes up. Let's say you have a long name of the chart and 10 tags. I would say that title is more important in this case. How tags would be truncated?
2. We are adding something called metadata bar to the header, which will display the most important info about the object. We could add a section "tags" which would display all of the tags on hover for example. Below screenshot on how the metadata bar will look like.
<img width="1306" alt="image" src="https://user-images.githubusercontent.com/36897697/181609940-297df6ca-16dc-4765-8bfa-7bb83541acd2.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] JohnDietrich-Pepper commented on pull request #20876: feat: Frontend tagging
Posted by "JohnDietrich-Pepper (via GitHub)" <gi...@apache.org>.
JohnDietrich-Pepper commented on PR #20876:
URL: https://github.com/apache/superset/pull/20876#issuecomment-1685610847
How come this didn't make it into 3.0?
--
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] lyndsiWilliams commented on pull request #20876: feat: Frontend tagging
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on PR #20876:
URL: https://github.com/apache/superset/pull/20876#issuecomment-1344729815
/testenv up TAGGING_SYSTEM=true
--
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] rusackas commented on pull request #20876: feat: Frontend tagging
Posted by GitBox <gi...@apache.org>.
rusackas commented on PR #20876:
URL: https://github.com/apache/superset/pull/20876#issuecomment-1344865146
> /testenv up TAGGING_SYSTEM=true
I think this needs to be `FEATURE_TAGGING_SYSTEM` for test envs
--
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] hughhhh commented on a diff in pull request #20876: feat: Frontend tagging
Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh commented on code in PR #20876:
URL: https://github.com/apache/superset/pull/20876#discussion_r1107672104
##########
superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx:
##########
@@ -355,6 +361,25 @@ function DashboardList(props: DashboardListProps) {
disableSortBy: true,
size: 'xl',
},
+ {
+ Cell: ({
+ row: {
+ original: { tags = [] },
+ },
+ }: any) => (
Review Comment:
can we define this interface?
--
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] eschutho commented on pull request #20876: feat: Frontend tagging
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on PR #20876:
URL: https://github.com/apache/superset/pull/20876#issuecomment-1435138140
Yey, congrats @cccs-RyanK, this is a really great feature. Are we clear to merge this on Tuesday?
--
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] lyndsiWilliams commented on a diff in pull request #20876: feat: Frontend tagging
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #20876:
URL: https://github.com/apache/superset/pull/20876#discussion_r1043856735
##########
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:
##########
@@ -350,6 +362,25 @@ const PropertiesModal = ({
updateMetadata: false,
});
+ if (isFeatureEnabled(FeatureFlag.TAGGING_SYSTEM)) {
+ // update tags
+ try {
+ fetchTags(
+ {
+ objectType: OBJECT_TYPES.DASHBOARD,
+ objectId: dashboardId,
+ includeTypes: false,
+ },
+ (currentTags: TagType[]) => updateTags(currentTags, tags),
+ error => {
+ handleErrorResponse(error);
+ },
+ );
+ } catch (error: any) {
Review Comment:
Can this `any` be defined?
##########
superset-frontend/src/components/Tags/TagsList.tsx:
##########
@@ -0,0 +1,112 @@
+/**
+ * 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, { useMemo, useState } from 'react';
+import { styled } from '@superset-ui/core';
+import TagType from 'src/types/TagType';
+import Tag from './Tag';
+
+export type TagsListProps = {
+ tags: TagType[];
+ editable?: boolean;
+ /**
+ * OnDelete:
+ * Only applies when editable is true
+ * Callback for when a tag is deleted
+ */
+ onDelete?: ((index: number) => void) | undefined;
+ maxTags?: number | undefined;
+};
+
+const TagsDiv = styled.div`
+ max-width: 100%;
+ display: flex;
+ flex-direction: row;
+ flex-wrap: wrap;
+`;
+
+const TagsList = ({
+ tags,
+ editable = false,
+ onDelete,
+ maxTags,
+}: TagsListProps) => {
+ const [tempMaxTags, setTempMaxTags] = useState<number | undefined>(maxTags);
+
+ const handleDelete = (index: number) => {
+ onDelete?.(index);
+ };
+
+ const expand = () => setTempMaxTags(undefined);
+
+ const collapse = () => setTempMaxTags(maxTags);
+
+ const tagsIsLong: boolean | null = useMemo(
+ () => (tempMaxTags ? tags.length > tempMaxTags : null),
+ [tags.length, tempMaxTags],
+ );
+
+ const extraTags: number | null = useMemo(
+ () =>
+ typeof tempMaxTags === 'number' ? tags.length - tempMaxTags + 1 : null,
+ [tagsIsLong, tags.length, tempMaxTags],
+ );
+
+ return (
+ <TagsDiv className="tag-list">
+ {tagsIsLong === true && typeof tempMaxTags === 'number' ? (
Review Comment:
```suggestion
{tagsIsLong && typeof tempMaxTags === 'number' ? (
```
This should still check for truthiness without `=== true`
##########
superset-frontend/src/views/CRUD/allentities/AllEntities.tsx:
##########
@@ -0,0 +1,95 @@
+/**
+ * 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, { useEffect, useState } from 'react';
+import { styled } from '@superset-ui/core';
+import Tag from 'src/types/TagType';
+import { StringParam, useQueryParam } from 'use-query-params';
+import withToasts from 'src/components/MessageToasts/withToasts';
+import SelectControl from 'src/explore/components/controls/SelectControl';
+import { fetchSuggestions } from 'src/tags';
+import AllEntitiesTable from './AllEntitiesTable';
+
+const AllEntitiesContainer = styled.div`
+ background-color: ${({ theme }) => theme.colors.grayscale.light4};
+ .select-control {
+ margin-left: ${({ theme }) => theme.gridUnit * 4}px;
+ margin-right: ${({ theme }) => theme.gridUnit * 4}px;
+ margin-bottom: ${({ theme }) => theme.gridUnit * 2}px;
+ }
+ .select-control-label {
+ text-transform: uppercase;
+ font-size: ${({ theme }) => theme.gridUnit * 3}px;
+ color: ${({ theme }) => theme.colors.grayscale.base};
+ margin-bottom: ${({ theme }) => theme.gridUnit * 1}px;
+ }
+`;
+
+const AllEntitiesNav = styled.div`
+ height: 50px;
+ background-color: ${({ theme }) => theme.colors.grayscale.light5};
+ margin-bottom: ${({ theme }) => theme.gridUnit * 4}px;
+ .navbar-brand {
+ margin-left: ${({ theme }) => theme.gridUnit * 2}px;
+ font-weight: ${({ theme }) => theme.typography.weights.bold};
+ }
+`;
Review Comment:
The superset theme's gridUnit should be used for the height value, also you can call theme once instead of multiple times by passing it in like this:
```suggestion
const AllEntitiesNav = styled.div`
${({ theme }) => `
height: ${theme.gridUnit * 12.5}px;
background-color: ${theme.colors.grayscale.light5};
margin-bottom: ${theme.gridUnit * 4}px;
.navbar-brand {
margin-left: ${theme.gridUnit * 2}px;
font-weight: ${theme.typography.weights.bold};
}
};
`;
```
##########
superset-frontend/src/views/CRUD/allentities/AllEntitiesTable.tsx:
##########
@@ -0,0 +1,121 @@
+/**
+ * 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, { useState, useEffect } from 'react';
+import moment from 'moment';
+import { t, styled } from '@superset-ui/core';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { fetchObjects } from '../../../tags';
+import Loading from '../../../components/Loading';
+
+const AllEntitiesTableContainer = styled.div`
+ text-align: left;
+ border-radius: ${({ theme }) => theme.gridUnit * 1}px 0;
+ margin: 0 ${({ theme }) => theme.gridUnit * 4}px;
+ .table {
+ table-layout: fixed;
+ }
+ .td {
+ width: 33%;
+ }
+`;
+
+interface TaggedObject {
+ id: number;
+ type: string;
+ name: string;
+ url: string;
+ changed_on: moment.MomentInput;
+ created_by: number | undefined;
+ creator: string;
+}
+
+interface TaggedObjects {
+ dashboard: TaggedObject[];
+ chart: TaggedObject[];
+ query: TaggedObject[];
+}
+
+interface AllEntitiesTableProps {
+ search?: string;
+}
+
+export default function AllEntitiesTable({
+ search = '',
+}: AllEntitiesTableProps) {
+ const [objects, setObjects] = useState<TaggedObjects>({
+ dashboard: [],
+ chart: [],
+ query: [],
+ });
+
+ useEffect(() => {
+ fetchObjects(
+ { tags: search, types: null },
+ (data: TaggedObject[]) => {
+ const objects = { dashboard: [], chart: [], query: [] };
+ data.forEach(object => {
+ objects[object.type].push(object);
+ });
+ setObjects(objects);
+ },
+ (error: Response) => {
+ console.log(error.json());
+ },
+ );
+ }, [search]);
+
+ const renderTable = (type: any) => {
+ const data = objects[type].map((o: TaggedObject) => ({
+ [type]: <a href={o.url}>{o.name}</a>,
+ // eslint-disable-next-line react/no-danger
+ modified: moment.utc(o.changed_on).fromNow(),
Review Comment:
Is there a way to implement this without triggering the `react/no-danger` lint rule?
##########
superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx:
##########
@@ -339,6 +347,25 @@ function DashboardList(props: DashboardListProps) {
disableSortBy: true,
size: 'xl',
},
+ {
+ Cell: ({
+ row: {
+ original: { tags = [] },
+ },
+ }: any) => (
Review Comment:
Resurfacing this, can this `any` be defined?
##########
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:
##########
@@ -350,6 +362,25 @@ const PropertiesModal = ({
updateMetadata: false,
});
+ if (isFeatureEnabled(FeatureFlag.TAGGING_SYSTEM)) {
+ // update tags
+ try {
+ fetchTags(
+ {
+ objectType: OBJECT_TYPES.DASHBOARD,
+ objectId: dashboardId,
+ includeTypes: false,
+ },
+ (currentTags: TagType[]) => updateTags(currentTags, tags),
+ error => {
+ handleErrorResponse(error);
+ },
+ );
+ } catch (error: any) {
+ console.log(error);
Review Comment:
There is a logging utility in superset-frontend that can be used here instead of using `console.log`:
`import { logging } from '@superset-ui/core';`
Then you can change this line to `logging.log(error);`
##########
superset-frontend/src/views/CRUD/allentities/AllEntities.tsx:
##########
@@ -0,0 +1,95 @@
+/**
+ * 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, { useEffect, useState } from 'react';
+import { styled } from '@superset-ui/core';
+import Tag from 'src/types/TagType';
+import { StringParam, useQueryParam } from 'use-query-params';
+import withToasts from 'src/components/MessageToasts/withToasts';
+import SelectControl from 'src/explore/components/controls/SelectControl';
+import { fetchSuggestions } from 'src/tags';
+import AllEntitiesTable from './AllEntitiesTable';
+
+const AllEntitiesContainer = styled.div`
+ background-color: ${({ theme }) => theme.colors.grayscale.light4};
+ .select-control {
+ margin-left: ${({ theme }) => theme.gridUnit * 4}px;
+ margin-right: ${({ theme }) => theme.gridUnit * 4}px;
+ margin-bottom: ${({ theme }) => theme.gridUnit * 2}px;
+ }
+ .select-control-label {
+ text-transform: uppercase;
+ font-size: ${({ theme }) => theme.gridUnit * 3}px;
+ color: ${({ theme }) => theme.colors.grayscale.base};
+ margin-bottom: ${({ theme }) => theme.gridUnit * 1}px;
+ }
+`;
Review Comment:
You can call theme once instead of multiple times by passing it in this way:
```suggestion
const AllEntitiesContainer = styled.div`
${({ theme }) => `
background-color: ${theme.colors.grayscale.light4};
.select-control {
margin-left: ${theme.gridUnit * 4}px;
margin-right: ${theme.gridUnit * 4}px;
margin-bottom: ${theme.gridUnit * 2}px;
}
.select-control-label {
text-transform: uppercase;
font-size: ${theme.gridUnit * 3}px;
color: ${theme.colors.grayscale.base};
margin-bottom: ${theme.gridUnit * 1}px;
}
};
`;
```
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -148,6 +167,25 @@ function PropertiesModal({
}[]
).map(o => o.value);
}
+ if (isFeatureEnabled(FeatureFlag.TAGGING_SYSTEM)) {
+ // update tags
+ try {
+ fetchTags(
+ {
+ objectType: OBJECT_TYPES.CHART,
+ objectId: slice.slice_id,
+ includeTypes: false,
+ },
+ (currentTags: TagType[]) => updateTags(currentTags, tags),
+ error => {
+ showError(error);
+ },
+ );
+ } catch (error: any) {
+ console.log(error);
Review Comment:
There is a logging utility in superset-frontend that can be used here instead of using `console.log`:
`import { logging } from '@superset-ui/core';`
Then you can change this line to `logging.log(error);`
##########
superset-frontend/src/views/CRUD/allentities/AllEntities.tsx:
##########
@@ -0,0 +1,95 @@
+/**
+ * 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, { useEffect, useState } from 'react';
+import { styled } from '@superset-ui/core';
+import Tag from 'src/types/TagType';
+import { StringParam, useQueryParam } from 'use-query-params';
+import withToasts from 'src/components/MessageToasts/withToasts';
+import SelectControl from 'src/explore/components/controls/SelectControl';
+import { fetchSuggestions } from 'src/tags';
+import AllEntitiesTable from './AllEntitiesTable';
+
+const AllEntitiesContainer = styled.div`
+ background-color: ${({ theme }) => theme.colors.grayscale.light4};
+ .select-control {
+ margin-left: ${({ theme }) => theme.gridUnit * 4}px;
+ margin-right: ${({ theme }) => theme.gridUnit * 4}px;
+ margin-bottom: ${({ theme }) => theme.gridUnit * 2}px;
+ }
+ .select-control-label {
+ text-transform: uppercase;
+ font-size: ${({ theme }) => theme.gridUnit * 3}px;
+ color: ${({ theme }) => theme.colors.grayscale.base};
+ margin-bottom: ${({ theme }) => theme.gridUnit * 1}px;
+ }
+`;
+
+const AllEntitiesNav = styled.div`
+ height: 50px;
+ background-color: ${({ theme }) => theme.colors.grayscale.light5};
+ margin-bottom: ${({ theme }) => theme.gridUnit * 4}px;
+ .navbar-brand {
+ margin-left: ${({ theme }) => theme.gridUnit * 2}px;
+ font-weight: ${({ theme }) => theme.typography.weights.bold};
+ }
+`;
+
+function AllEntities() {
+ const [tagSuggestions, setTagSuggestions] = useState<string[]>();
+ const [tagsQuery, setTagsQuery] = useQueryParam('tags', StringParam);
+
+ useEffect(() => {
+ fetchSuggestions(
+ { includeTypes: false },
+ (suggestions: Tag[]) => {
+ const tagSuggestions = [...suggestions.map(tag => tag.name)];
+ setTagSuggestions(tagSuggestions);
+ },
+ (error: Response) => {
+ console.log(error.json());
+ },
+ );
+ }, [tagsQuery]);
+
+ const onTagSearchChange = (tags: Tag[]) => {
+ const tagSearch = tags.join(',');
+ setTagsQuery(tagSearch);
+ };
+
+ return (
+ <AllEntitiesContainer>
+ <AllEntitiesNav>
+ <span className="navbar-brand">All Entities</span>
+ </AllEntitiesNav>
+ <div className="select-control">
+ <div className="select-control-label">search by tags</div>
Review Comment:
These lines need translation: `import { t } from '@superset-ui/core';`
```suggestion
<span className="navbar-brand">{t('All Entities')}</span>
</AllEntitiesNav>
<div className="select-control">
<div className="select-control-label">{t('search by tags')}</div>
```
##########
superset-frontend/src/components/Tags/Tag.tsx:
##########
@@ -0,0 +1,84 @@
+/**
+ * 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 { styled } from '@superset-ui/core';
+import TagType from 'src/types/TagType';
+import AntdTag from 'antd/lib/tag';
+import React, { useMemo } from 'react';
+import { Tooltip } from 'src/components/Tooltip';
+
+const StyledTag = styled(AntdTag)`
+ margin-top: ${({ theme }) => theme.gridUnit}px;
+ margin-bottom: ${({ theme }) => theme.gridUnit}px;
+ font-size: ${({ theme }) => theme.typography.sizes.s}px;
+`;
Review Comment:
You can call theme once instead of multiple times by passing it in like this:
```suggestion
const StyledTag = styled(AntdTag)`
${({ theme }) => `
margin-top: ${theme.gridUnit}px;
margin-bottom: ${theme.gridUnit}px;
font-size: ${theme.typography.sizes.s}px;
};
`;
```
##########
superset-frontend/src/views/CRUD/allentities/AllEntitiesTable.tsx:
##########
@@ -0,0 +1,121 @@
+/**
+ * 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, { useState, useEffect } from 'react';
+import moment from 'moment';
+import { t, styled } from '@superset-ui/core';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { fetchObjects } from '../../../tags';
+import Loading from '../../../components/Loading';
+
+const AllEntitiesTableContainer = styled.div`
+ text-align: left;
+ border-radius: ${({ theme }) => theme.gridUnit * 1}px 0;
+ margin: 0 ${({ theme }) => theme.gridUnit * 4}px;
+ .table {
+ table-layout: fixed;
+ }
+ .td {
+ width: 33%;
+ }
+`;
+
+interface TaggedObject {
+ id: number;
+ type: string;
+ name: string;
+ url: string;
+ changed_on: moment.MomentInput;
+ created_by: number | undefined;
+ creator: string;
+}
+
+interface TaggedObjects {
+ dashboard: TaggedObject[];
+ chart: TaggedObject[];
+ query: TaggedObject[];
+}
+
+interface AllEntitiesTableProps {
+ search?: string;
+}
+
+export default function AllEntitiesTable({
+ search = '',
+}: AllEntitiesTableProps) {
+ const [objects, setObjects] = useState<TaggedObjects>({
+ dashboard: [],
+ chart: [],
+ query: [],
+ });
+
+ useEffect(() => {
+ fetchObjects(
+ { tags: search, types: null },
+ (data: TaggedObject[]) => {
+ const objects = { dashboard: [], chart: [], query: [] };
+ data.forEach(object => {
+ objects[object.type].push(object);
+ });
+ setObjects(objects);
+ },
+ (error: Response) => {
+ console.log(error.json());
+ },
+ );
+ }, [search]);
+
+ const renderTable = (type: any) => {
Review Comment:
Can this `any` be defined?
##########
superset-frontend/src/types/TagType.ts:
##########
@@ -0,0 +1,30 @@
+/**
+ * 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.
+ */
+
+export interface TagType {
+ id?: string | number;
+ type?: string | number;
+ editable?: boolean;
+ onDelete?: any;
+ onClick?: any;
Review Comment:
Can these `any`s be defined?
##########
superset-frontend/src/views/CRUD/allentities/AllEntities.tsx:
##########
@@ -0,0 +1,95 @@
+/**
+ * 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, { useEffect, useState } from 'react';
+import { styled } from '@superset-ui/core';
+import Tag from 'src/types/TagType';
+import { StringParam, useQueryParam } from 'use-query-params';
+import withToasts from 'src/components/MessageToasts/withToasts';
+import SelectControl from 'src/explore/components/controls/SelectControl';
+import { fetchSuggestions } from 'src/tags';
+import AllEntitiesTable from './AllEntitiesTable';
+
+const AllEntitiesContainer = styled.div`
+ background-color: ${({ theme }) => theme.colors.grayscale.light4};
+ .select-control {
+ margin-left: ${({ theme }) => theme.gridUnit * 4}px;
+ margin-right: ${({ theme }) => theme.gridUnit * 4}px;
+ margin-bottom: ${({ theme }) => theme.gridUnit * 2}px;
+ }
+ .select-control-label {
+ text-transform: uppercase;
+ font-size: ${({ theme }) => theme.gridUnit * 3}px;
+ color: ${({ theme }) => theme.colors.grayscale.base};
+ margin-bottom: ${({ theme }) => theme.gridUnit * 1}px;
+ }
+`;
+
+const AllEntitiesNav = styled.div`
+ height: 50px;
+ background-color: ${({ theme }) => theme.colors.grayscale.light5};
+ margin-bottom: ${({ theme }) => theme.gridUnit * 4}px;
+ .navbar-brand {
+ margin-left: ${({ theme }) => theme.gridUnit * 2}px;
+ font-weight: ${({ theme }) => theme.typography.weights.bold};
+ }
+`;
+
+function AllEntities() {
+ const [tagSuggestions, setTagSuggestions] = useState<string[]>();
+ const [tagsQuery, setTagsQuery] = useQueryParam('tags', StringParam);
+
+ useEffect(() => {
+ fetchSuggestions(
+ { includeTypes: false },
+ (suggestions: Tag[]) => {
+ const tagSuggestions = [...suggestions.map(tag => tag.name)];
+ setTagSuggestions(tagSuggestions);
+ },
+ (error: Response) => {
+ console.log(error.json());
Review Comment:
There is a logging utility in superset-frontend that can be used here instead of using `console.log`:
`import { logging } from '@superset-ui/core';`
Then you can change this line to `logging.log(error.json());`
##########
superset-frontend/src/views/CRUD/allentities/AllEntitiesTable.tsx:
##########
@@ -0,0 +1,121 @@
+/**
+ * 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, { useState, useEffect } from 'react';
+import moment from 'moment';
+import { t, styled } from '@superset-ui/core';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { fetchObjects } from '../../../tags';
+import Loading from '../../../components/Loading';
+
+const AllEntitiesTableContainer = styled.div`
+ text-align: left;
+ border-radius: ${({ theme }) => theme.gridUnit * 1}px 0;
+ margin: 0 ${({ theme }) => theme.gridUnit * 4}px;
+ .table {
+ table-layout: fixed;
+ }
+ .td {
+ width: 33%;
+ }
+`;
+
+interface TaggedObject {
+ id: number;
+ type: string;
+ name: string;
+ url: string;
+ changed_on: moment.MomentInput;
+ created_by: number | undefined;
+ creator: string;
+}
+
+interface TaggedObjects {
+ dashboard: TaggedObject[];
+ chart: TaggedObject[];
+ query: TaggedObject[];
+}
+
+interface AllEntitiesTableProps {
+ search?: string;
+}
+
+export default function AllEntitiesTable({
+ search = '',
+}: AllEntitiesTableProps) {
+ const [objects, setObjects] = useState<TaggedObjects>({
+ dashboard: [],
+ chart: [],
+ query: [],
+ });
+
+ useEffect(() => {
+ fetchObjects(
+ { tags: search, types: null },
+ (data: TaggedObject[]) => {
+ const objects = { dashboard: [], chart: [], query: [] };
+ data.forEach(object => {
+ objects[object.type].push(object);
+ });
+ setObjects(objects);
+ },
+ (error: Response) => {
+ console.log(error.json());
Review Comment:
There is a logging utility in superset-frontend that can be used here instead of using `console.log`:
`import { logging } from '@superset-ui/core';`
Then you can change this line to `logging.log(error.json());`
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -183,6 +222,72 @@ function PropertiesModal({
setName(slice.slice_name || '');
}, [slice.slice_name]);
+ useEffect(() => {
+ if (!isFeatureEnabled(FeatureFlag.TAGGING_SYSTEM)) return;
+ try {
+ fetchTags(
+ {
+ objectType: OBJECT_TYPES.CHART,
+ objectId: slice.slice_id,
+ includeTypes: false,
+ },
+ (tags: TagType[]) => setTags(tags),
+ error => {
+ showError(error);
+ },
+ );
+ } catch (error: any) {
+ console.log(error);
Review Comment:
There is a logging utility in superset-frontend that can be used here instead of using `console.log`:
`import { logging } from '@superset-ui/core';`
Then you can change this line to `logging.log(error);`
##########
superset-frontend/src/views/CRUD/tags/TagList.tsx:
##########
@@ -0,0 +1,332 @@
+/**
+ * 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 { t } from '@superset-ui/core';
+import React, { useMemo, useCallback } from 'react';
+import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags';
+import {
+ createFetchRelated,
+ createErrorHandler,
+ Actions,
+} from 'src/views/CRUD/utils';
+import { useListViewResource } from 'src/views/CRUD/hooks';
+import ConfirmStatusChange from 'src/components/ConfirmStatusChange';
+import SubMenu, { SubMenuProps } from 'src/views/components/SubMenu';
+import ListView, {
+ ListViewProps,
+ Filters,
+ FilterOperator,
+} from 'src/components/ListView';
+import { dangerouslyGetItemDoNotUse } from 'src/utils/localStorageHelpers';
+import withToasts from 'src/components/MessageToasts/withToasts';
+import Icons from 'src/components/Icons';
+import { Tooltip } from 'src/components/Tooltip';
+import FacePile from 'src/components/FacePile';
+import { Link } from 'react-router-dom';
+import { deleteTags } from 'src/tags';
+import { Tag as AntdTag } from 'antd';
+import { Tag } from '../types';
+import TagCard from './TagCard';
+
+const PAGE_SIZE = 25;
+
+interface TagListProps {
+ addDangerToast: (msg: string) => void;
+ addSuccessToast: (msg: string) => void;
+ user: {
+ userId: string | number;
+ firstName: string;
+ lastName: string;
+ };
+}
+
+function TagList(props: TagListProps) {
+ const {
+ addDangerToast,
+ addSuccessToast,
+ user: { userId },
+ } = props;
+
+ const {
+ state: {
+ loading,
+ resourceCount: tagCount,
+ resourceCollection: tags,
+ bulkSelectEnabled,
+ },
+ hasPerm,
+ fetchData,
+ toggleBulkSelect,
+ refreshData,
+ } = useListViewResource<Tag>('tag', t('tag'), addDangerToast);
+
+ // TODO: Fix usage of localStorage keying on the user id
+ const userKey = dangerouslyGetItemDoNotUse(userId?.toString(), null);
Review Comment:
This function seems a little scary to use, is there another way that this can be done?
##########
superset-frontend/src/views/CRUD/tags/TagList.tsx:
##########
@@ -0,0 +1,332 @@
+/**
+ * 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 { t } from '@superset-ui/core';
+import React, { useMemo, useCallback } from 'react';
+import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags';
+import {
+ createFetchRelated,
+ createErrorHandler,
+ Actions,
+} from 'src/views/CRUD/utils';
+import { useListViewResource } from 'src/views/CRUD/hooks';
+import ConfirmStatusChange from 'src/components/ConfirmStatusChange';
+import SubMenu, { SubMenuProps } from 'src/views/components/SubMenu';
+import ListView, {
+ ListViewProps,
+ Filters,
+ FilterOperator,
+} from 'src/components/ListView';
+import { dangerouslyGetItemDoNotUse } from 'src/utils/localStorageHelpers';
+import withToasts from 'src/components/MessageToasts/withToasts';
+import Icons from 'src/components/Icons';
+import { Tooltip } from 'src/components/Tooltip';
+import FacePile from 'src/components/FacePile';
+import { Link } from 'react-router-dom';
+import { deleteTags } from 'src/tags';
+import { Tag as AntdTag } from 'antd';
+import { Tag } from '../types';
+import TagCard from './TagCard';
+
+const PAGE_SIZE = 25;
+
+interface TagListProps {
+ addDangerToast: (msg: string) => void;
+ addSuccessToast: (msg: string) => void;
+ user: {
+ userId: string | number;
+ firstName: string;
+ lastName: string;
+ };
+}
+
+function TagList(props: TagListProps) {
+ const {
+ addDangerToast,
+ addSuccessToast,
+ user: { userId },
+ } = props;
+
+ const {
+ state: {
+ loading,
+ resourceCount: tagCount,
+ resourceCollection: tags,
+ bulkSelectEnabled,
+ },
+ hasPerm,
+ fetchData,
+ toggleBulkSelect,
+ refreshData,
+ } = useListViewResource<Tag>('tag', t('tag'), addDangerToast);
+
+ // TODO: Fix usage of localStorage keying on the user id
+ const userKey = dangerouslyGetItemDoNotUse(userId?.toString(), null);
+
+ const canDelete = hasPerm('can_write');
+
+ const initialSort = [{ id: 'changed_on_delta_humanized', desc: true }];
+
+ function handleTagsDelete(
+ tags: Tag[],
+ callback: (text: string) => void,
+ error: (text: string) => void,
+ ) {
+ // TODO what permissions need to be checked here?
+ deleteTags(tags, callback, error);
+ refreshData();
+ }
+
+ const columns = useMemo(
+ () => [
+ {
+ Cell: ({
+ row: {
+ original: { name: tagName },
+ },
+ }: any) => (
+ <AntdTag>
+ <Link to={`/superset/all_entities/?tags=${tagName}`}>
+ {tagName}
+ </Link>
+ </AntdTag>
+ ),
+ Header: t('Name'),
+ accessor: 'name',
+ },
+ {
+ Cell: ({
+ row: {
+ original: { changed_on_delta_humanized: changedOn },
+ },
+ }: any) => <span className="no-wrap">{changedOn}</span>,
+ Header: t('Modified'),
+ accessor: 'changed_on_delta_humanized',
+ size: 'xl',
+ },
+ {
+ Cell: ({
+ row: {
+ original: { created_by: createdBy },
+ },
+ }: any) => (createdBy ? <FacePile users={[createdBy]} /> : ''),
+ Header: t('Created by'),
+ accessor: 'created_by',
+ disableSortBy: true,
+ size: 'xl',
+ },
+ {
+ Cell: ({ row: { original } }: any) => {
Review Comment:
Can this `any` be defined?
##########
superset-frontend/src/views/CRUD/tags/TagList.tsx:
##########
@@ -0,0 +1,332 @@
+/**
+ * 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 { t } from '@superset-ui/core';
+import React, { useMemo, useCallback } from 'react';
+import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags';
+import {
+ createFetchRelated,
+ createErrorHandler,
+ Actions,
+} from 'src/views/CRUD/utils';
+import { useListViewResource } from 'src/views/CRUD/hooks';
+import ConfirmStatusChange from 'src/components/ConfirmStatusChange';
+import SubMenu, { SubMenuProps } from 'src/views/components/SubMenu';
+import ListView, {
+ ListViewProps,
+ Filters,
+ FilterOperator,
+} from 'src/components/ListView';
+import { dangerouslyGetItemDoNotUse } from 'src/utils/localStorageHelpers';
+import withToasts from 'src/components/MessageToasts/withToasts';
+import Icons from 'src/components/Icons';
+import { Tooltip } from 'src/components/Tooltip';
+import FacePile from 'src/components/FacePile';
+import { Link } from 'react-router-dom';
+import { deleteTags } from 'src/tags';
+import { Tag as AntdTag } from 'antd';
+import { Tag } from '../types';
+import TagCard from './TagCard';
+
+const PAGE_SIZE = 25;
+
+interface TagListProps {
+ addDangerToast: (msg: string) => void;
+ addSuccessToast: (msg: string) => void;
+ user: {
+ userId: string | number;
+ firstName: string;
+ lastName: string;
+ };
+}
+
+function TagList(props: TagListProps) {
+ const {
+ addDangerToast,
+ addSuccessToast,
+ user: { userId },
+ } = props;
+
+ const {
+ state: {
+ loading,
+ resourceCount: tagCount,
+ resourceCollection: tags,
+ bulkSelectEnabled,
+ },
+ hasPerm,
+ fetchData,
+ toggleBulkSelect,
+ refreshData,
+ } = useListViewResource<Tag>('tag', t('tag'), addDangerToast);
+
+ // TODO: Fix usage of localStorage keying on the user id
+ const userKey = dangerouslyGetItemDoNotUse(userId?.toString(), null);
+
+ const canDelete = hasPerm('can_write');
+
+ const initialSort = [{ id: 'changed_on_delta_humanized', desc: true }];
+
+ function handleTagsDelete(
+ tags: Tag[],
+ callback: (text: string) => void,
+ error: (text: string) => void,
+ ) {
+ // TODO what permissions need to be checked here?
+ deleteTags(tags, callback, error);
+ refreshData();
+ }
+
+ const columns = useMemo(
+ () => [
+ {
+ Cell: ({
+ row: {
+ original: { name: tagName },
+ },
+ }: any) => (
+ <AntdTag>
+ <Link to={`/superset/all_entities/?tags=${tagName}`}>
+ {tagName}
+ </Link>
+ </AntdTag>
+ ),
+ Header: t('Name'),
+ accessor: 'name',
+ },
+ {
+ Cell: ({
+ row: {
+ original: { changed_on_delta_humanized: changedOn },
+ },
+ }: any) => <span className="no-wrap">{changedOn}</span>,
Review Comment:
Can this `any` be defined?
##########
superset-frontend/src/views/CRUD/tags/TagList.tsx:
##########
@@ -0,0 +1,332 @@
+/**
+ * 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 { t } from '@superset-ui/core';
+import React, { useMemo, useCallback } from 'react';
+import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags';
+import {
+ createFetchRelated,
+ createErrorHandler,
+ Actions,
+} from 'src/views/CRUD/utils';
+import { useListViewResource } from 'src/views/CRUD/hooks';
+import ConfirmStatusChange from 'src/components/ConfirmStatusChange';
+import SubMenu, { SubMenuProps } from 'src/views/components/SubMenu';
+import ListView, {
+ ListViewProps,
+ Filters,
+ FilterOperator,
+} from 'src/components/ListView';
+import { dangerouslyGetItemDoNotUse } from 'src/utils/localStorageHelpers';
+import withToasts from 'src/components/MessageToasts/withToasts';
+import Icons from 'src/components/Icons';
+import { Tooltip } from 'src/components/Tooltip';
+import FacePile from 'src/components/FacePile';
+import { Link } from 'react-router-dom';
+import { deleteTags } from 'src/tags';
+import { Tag as AntdTag } from 'antd';
+import { Tag } from '../types';
+import TagCard from './TagCard';
+
+const PAGE_SIZE = 25;
+
+interface TagListProps {
+ addDangerToast: (msg: string) => void;
+ addSuccessToast: (msg: string) => void;
+ user: {
+ userId: string | number;
+ firstName: string;
+ lastName: string;
+ };
+}
+
+function TagList(props: TagListProps) {
+ const {
+ addDangerToast,
+ addSuccessToast,
+ user: { userId },
+ } = props;
+
+ const {
+ state: {
+ loading,
+ resourceCount: tagCount,
+ resourceCollection: tags,
+ bulkSelectEnabled,
+ },
+ hasPerm,
+ fetchData,
+ toggleBulkSelect,
+ refreshData,
+ } = useListViewResource<Tag>('tag', t('tag'), addDangerToast);
+
+ // TODO: Fix usage of localStorage keying on the user id
+ const userKey = dangerouslyGetItemDoNotUse(userId?.toString(), null);
+
+ const canDelete = hasPerm('can_write');
+
+ const initialSort = [{ id: 'changed_on_delta_humanized', desc: true }];
+
+ function handleTagsDelete(
+ tags: Tag[],
+ callback: (text: string) => void,
+ error: (text: string) => void,
+ ) {
+ // TODO what permissions need to be checked here?
+ deleteTags(tags, callback, error);
+ refreshData();
+ }
+
+ const columns = useMemo(
+ () => [
+ {
+ Cell: ({
+ row: {
+ original: { name: tagName },
+ },
+ }: any) => (
Review Comment:
Can this `any` be defined?
##########
superset-frontend/src/views/CRUD/tags/TagList.tsx:
##########
@@ -0,0 +1,332 @@
+/**
+ * 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 { t } from '@superset-ui/core';
+import React, { useMemo, useCallback } from 'react';
+import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags';
+import {
+ createFetchRelated,
+ createErrorHandler,
+ Actions,
+} from 'src/views/CRUD/utils';
+import { useListViewResource } from 'src/views/CRUD/hooks';
+import ConfirmStatusChange from 'src/components/ConfirmStatusChange';
+import SubMenu, { SubMenuProps } from 'src/views/components/SubMenu';
+import ListView, {
+ ListViewProps,
+ Filters,
+ FilterOperator,
+} from 'src/components/ListView';
+import { dangerouslyGetItemDoNotUse } from 'src/utils/localStorageHelpers';
+import withToasts from 'src/components/MessageToasts/withToasts';
+import Icons from 'src/components/Icons';
+import { Tooltip } from 'src/components/Tooltip';
+import FacePile from 'src/components/FacePile';
+import { Link } from 'react-router-dom';
+import { deleteTags } from 'src/tags';
+import { Tag as AntdTag } from 'antd';
+import { Tag } from '../types';
+import TagCard from './TagCard';
+
+const PAGE_SIZE = 25;
+
+interface TagListProps {
+ addDangerToast: (msg: string) => void;
+ addSuccessToast: (msg: string) => void;
+ user: {
+ userId: string | number;
+ firstName: string;
+ lastName: string;
+ };
+}
+
+function TagList(props: TagListProps) {
+ const {
+ addDangerToast,
+ addSuccessToast,
+ user: { userId },
+ } = props;
+
+ const {
+ state: {
+ loading,
+ resourceCount: tagCount,
+ resourceCollection: tags,
+ bulkSelectEnabled,
+ },
+ hasPerm,
+ fetchData,
+ toggleBulkSelect,
+ refreshData,
+ } = useListViewResource<Tag>('tag', t('tag'), addDangerToast);
+
+ // TODO: Fix usage of localStorage keying on the user id
+ const userKey = dangerouslyGetItemDoNotUse(userId?.toString(), null);
+
+ const canDelete = hasPerm('can_write');
+
+ const initialSort = [{ id: 'changed_on_delta_humanized', desc: true }];
+
+ function handleTagsDelete(
+ tags: Tag[],
+ callback: (text: string) => void,
+ error: (text: string) => void,
+ ) {
+ // TODO what permissions need to be checked here?
Review Comment:
Will this TODO be addressed in this PR or a future one?
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -148,6 +170,25 @@ function PropertiesModal({
}[]
).map(o => o.value);
}
+ if (isFeatureEnabled(FeatureFlag.TAGGING_SYSTEM)) {
+ // update tags
+ try {
+ fetchTags(
+ {
+ objectType: OBJECT_TYPES.CHART,
+ objectId: slice.slice_id,
+ includeTypes: false,
+ },
+ (currentTags: TagType[]) => updateTags(currentTags, tags),
+ () => {
+ /* TODO: handle error */
+ },
+ );
+ } catch (error: any) {
Review Comment:
Resurfacing this, can this `any` be defined?
##########
superset-frontend/src/views/CRUD/tags/TagList.tsx:
##########
@@ -0,0 +1,332 @@
+/**
+ * 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 { t } from '@superset-ui/core';
+import React, { useMemo, useCallback } from 'react';
+import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags';
+import {
+ createFetchRelated,
+ createErrorHandler,
+ Actions,
+} from 'src/views/CRUD/utils';
+import { useListViewResource } from 'src/views/CRUD/hooks';
+import ConfirmStatusChange from 'src/components/ConfirmStatusChange';
+import SubMenu, { SubMenuProps } from 'src/views/components/SubMenu';
+import ListView, {
+ ListViewProps,
+ Filters,
+ FilterOperator,
+} from 'src/components/ListView';
+import { dangerouslyGetItemDoNotUse } from 'src/utils/localStorageHelpers';
+import withToasts from 'src/components/MessageToasts/withToasts';
+import Icons from 'src/components/Icons';
+import { Tooltip } from 'src/components/Tooltip';
+import FacePile from 'src/components/FacePile';
+import { Link } from 'react-router-dom';
+import { deleteTags } from 'src/tags';
+import { Tag as AntdTag } from 'antd';
+import { Tag } from '../types';
+import TagCard from './TagCard';
+
+const PAGE_SIZE = 25;
+
+interface TagListProps {
+ addDangerToast: (msg: string) => void;
+ addSuccessToast: (msg: string) => void;
+ user: {
+ userId: string | number;
+ firstName: string;
+ lastName: string;
+ };
+}
+
+function TagList(props: TagListProps) {
+ const {
+ addDangerToast,
+ addSuccessToast,
+ user: { userId },
+ } = props;
+
+ const {
+ state: {
+ loading,
+ resourceCount: tagCount,
+ resourceCollection: tags,
+ bulkSelectEnabled,
+ },
+ hasPerm,
+ fetchData,
+ toggleBulkSelect,
+ refreshData,
+ } = useListViewResource<Tag>('tag', t('tag'), addDangerToast);
+
+ // TODO: Fix usage of localStorage keying on the user id
+ const userKey = dangerouslyGetItemDoNotUse(userId?.toString(), null);
+
+ const canDelete = hasPerm('can_write');
+
+ const initialSort = [{ id: 'changed_on_delta_humanized', desc: true }];
+
+ function handleTagsDelete(
+ tags: Tag[],
+ callback: (text: string) => void,
+ error: (text: string) => void,
+ ) {
+ // TODO what permissions need to be checked here?
+ deleteTags(tags, callback, error);
+ refreshData();
+ }
+
+ const columns = useMemo(
+ () => [
+ {
+ Cell: ({
+ row: {
+ original: { name: tagName },
+ },
+ }: any) => (
+ <AntdTag>
+ <Link to={`/superset/all_entities/?tags=${tagName}`}>
+ {tagName}
+ </Link>
+ </AntdTag>
+ ),
+ Header: t('Name'),
+ accessor: 'name',
+ },
+ {
+ Cell: ({
+ row: {
+ original: { changed_on_delta_humanized: changedOn },
+ },
+ }: any) => <span className="no-wrap">{changedOn}</span>,
+ Header: t('Modified'),
+ accessor: 'changed_on_delta_humanized',
+ size: 'xl',
+ },
+ {
+ Cell: ({
+ row: {
+ original: { created_by: createdBy },
+ },
+ }: any) => (createdBy ? <FacePile users={[createdBy]} /> : ''),
+ Header: t('Created by'),
+ accessor: 'created_by',
+ disableSortBy: true,
+ size: 'xl',
+ },
+ {
+ Cell: ({ row: { original } }: any) => {
+ const handleDelete = () =>
+ handleTagsDelete([original], addSuccessToast, addDangerToast);
+ return (
+ <Actions className="actions">
+ {canDelete && (
+ <ConfirmStatusChange
+ title={t('Please confirm')}
+ description={
+ <>
+ {t('Are you sure you want to delete')}{' '}
+ <b>{original.dashboard_title}</b>?
+ </>
+ }
+ onConfirm={handleDelete}
+ >
+ {confirmDelete => (
+ <Tooltip
+ id="delete-action-tooltip"
+ title={t('Delete')}
+ placement="bottom"
+ >
+ <span
+ role="button"
+ tabIndex={0}
+ className="action-button"
+ onClick={confirmDelete}
+ >
+ {/* fix icon name */}
Review Comment:
Will this comment be addressed in this PR or a future one?
##########
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:
##########
@@ -521,6 +557,67 @@ const PropertiesModal = ({
}
}, [dashboardInfo, dashboardTitle, form]);
+ useEffect(() => {
+ if (!isFeatureEnabled(FeatureFlag.TAGGING_SYSTEM)) return;
+ try {
+ fetchTags(
+ {
+ objectType: OBJECT_TYPES.DASHBOARD,
+ objectId: dashboardId,
+ includeTypes: false,
+ },
+ (tags: TagType[]) => setTags(tags),
+ () => {
+ /* TODO: handle error */
+ },
+ );
+ } catch (error: any) {
Review Comment:
Resurfacing this, can this `any` be defined?
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -314,6 +421,26 @@ function PropertiesModal({
)}
</StyledHelpBlock>
</FormItem>
+ {isFeatureEnabled(FeatureFlag.TAGGING_SYSTEM) && (
+ <h3 style={{ marginTop: '1em' }}>{t('Tags')}</h3>
Review Comment:
Resurfacing this, `css` is used for inline styles as per [the frontend code style guidelines](https://github.com/apache/superset/wiki/Frontend-Style-Guidelines).
##########
superset-frontend/src/views/CRUD/chart/ChartList.tsx:
##########
@@ -366,6 +370,27 @@ function ChartList(props: ChartListProps) {
disableSortBy: true,
size: 'xl',
},
+ {
+ Cell: ({
+ row: {
+ original: { tags = [] },
+ },
+ }: any) => (
Review Comment:
Resurfacing this, can this `any` be defined?
##########
superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.tsx:
##########
@@ -362,6 +364,20 @@ function SavedQueryList({
accessor: 'changed_on_delta_humanized',
size: 'xl',
},
+ {
+ Cell: ({
+ row: {
+ original: { tags = [] },
+ },
+ }: any) => (
Review Comment:
Resurfacing this, can this `any` be defined?
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -182,6 +224,71 @@ function PropertiesModal({
setName(slice.slice_name || '');
}, [slice.slice_name]);
+ useEffect(() => {
+ if (!isFeatureEnabled(FeatureFlag.TAGGING_SYSTEM)) return;
+ try {
+ fetchTags(
+ {
+ objectType: OBJECT_TYPES.CHART,
+ objectId: slice.slice_id,
+ includeTypes: false,
+ },
+ (tags: TagType[]) => setTags(tags),
+ () => {
+ /* TODO: handle error */
+ },
+ );
+ } catch (error: any) {
Review Comment:
Resurfacing this, can this `any` be defined?
--
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] hughhhh commented on a diff in pull request #20876: feat: Frontend tagging
Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh commented on code in PR #20876:
URL: https://github.com/apache/superset/pull/20876#discussion_r1086944341
##########
superset/views/all_entities.py:
##########
@@ -0,0 +1,262 @@
+# 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.
+from __future__ import absolute_import, division, print_function, unicode_literals
+
+import logging
+from typing import Any, Dict, List
+
+import simplejson as json
+from flask import request, Response
+from flask_appbuilder import expose
+from flask_appbuilder.hooks import before_request
+from flask_appbuilder.models.sqla.interface import SQLAInterface
+from flask_appbuilder.security.decorators import has_access, has_access_api
+from jinja2.sandbox import SandboxedEnvironment
+from sqlalchemy import and_, func
+from werkzeug.exceptions import NotFound
+
+from superset import db, is_feature_enabled, utils
+from superset.jinja_context import ExtraCache
+from superset.models.dashboard import Dashboard
+from superset.models.slice import Slice
+from superset.models.sql_lab import SavedQuery
+from superset.superset_typing import FlaskResponse
+from superset.tags.commands.create import CreateCustomTagCommand
+from superset.tags.commands.exceptions import TagCreateFailedError, TagInvalidError
+from superset.tags.models import ObjectTypes, Tag, TaggedObject
+from superset.views.base import SupersetModelView
+
+from .base import BaseSupersetView, json_success
+
+logger = logging.getLogger(__name__)
+
+
+def process_template(content: str) -> str:
+ env = SandboxedEnvironment()
+ template = env.from_string(content)
+ context = {
+ "current_user_id": ExtraCache.current_user_id,
+ "current_username": ExtraCache.current_username,
+ }
+ return template.render(context)
+
+
+class TaggedObjectsModelView(SupersetModelView):
+ route_base = "/superset/all_entities"
+ datamodel = SQLAInterface(Tag)
+ class_permission_name = "Tags"
+
+ @has_access
+ @expose("/")
+ def list(self) -> FlaskResponse:
+ if not is_feature_enabled("TAGGING_SYSTEM"):
+ return super().list()
+
+ return super().render_app_template()
+
+
+class TaggedObjectView(BaseSupersetView):
Review Comment:
can we move these endpoints under `tags/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] hughhhh commented on a diff in pull request #20876: feat: Frontend tagging
Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh commented on code in PR #20876:
URL: https://github.com/apache/superset/pull/20876#discussion_r1088027498
##########
superset/tags/dao.py:
##########
@@ -0,0 +1,64 @@
+# 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 logging
+from typing import Any, Dict
+
+from superset.dao.base import BaseDAO
+from superset.dao.exceptions import DAOCreateFailedError
+from superset.extensions import db
+from superset.tags.models import ObjectTypes, Tag, TaggedObject, TagTypes
+
+logger = logging.getLogger(__name__)
+
+
+class TagDAO(BaseDAO):
+ model_cls = Tag
+ # base_filter = TagAccessFilter
+ @staticmethod
+ def validate_tag_name(tag_name: str) -> bool:
+ if ":" in tag_name:
+ return False
+ return True
+
+ @staticmethod
+ def create_custom_tagged_objects(
+ object_type: ObjectTypes, object_id: int, properties: Dict[str, Any]
+ ) -> None:
+ tag_names = properties["tags"]
+
+ tagged_objects = []
+ for name in tag_names:
+ if not TagDAO.validate_tag_name(name):
+ raise DAOCreateFailedError(
+ message="Invalid Tag Name (cannot contain ':')"
+ )
+ type_ = TagTypes.custom
+ tag_name = name.strip()
+ tag = TagDAO.get_by_name(tag_name, type_)
+ tagged_objects.append(
+ TaggedObject(object_id=object_id, object_type=object_type, tag=tag)
+ )
+
+ db.session.add_all(tagged_objects)
+ db.session.commit()
+
+ @staticmethod
+ def get_by_name(name: str, type_: TagTypes) -> Tag:
Review Comment:
another test here would be good
--
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] hughhhh commented on a diff in pull request #20876: feat: Frontend tagging
Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh commented on code in PR #20876:
URL: https://github.com/apache/superset/pull/20876#discussion_r1107669240
##########
superset-frontend/src/tags.ts:
##########
@@ -0,0 +1,187 @@
+/**
+ * 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 { JsonObject, SupersetClient } from '@superset-ui/core';
+import rison from 'rison';
+import Tag from 'src/types/TagType';
+
+export const OBJECT_TYPES_VALUES = Object.freeze([
+ 'dashboard',
+ 'chart',
+ 'saved_query',
+]);
+
+export const OBJECT_TYPES = Object.freeze({
+ DASHBOARD: 'dashboard',
+ CHART: 'chart',
+ QUERY: 'saved_query',
+});
+
+const OBJECT_TYPE_ID_MAP = {
+ saved_query: 1,
+ chart: 2,
+ dashboard: 3,
+};
+
+const map_object_type_to_id = (objectType: string) => {
+ if (!OBJECT_TYPES_VALUES.includes(objectType)) {
+ const msg = `objectType ${objectType} is invalid`;
+ throw new Error(msg);
+ }
+ return OBJECT_TYPE_ID_MAP[objectType];
+};
+
+export function fetchAllTags(
+ callback: (json: JsonObject) => void,
+ error: (response: Response) => void,
+) {
+ const url = `/api/v1/tag`;
+ SupersetClient.get({ endpoint: url })
Review Comment:
nit: just put the
```suggestion
SupersetClient.get({ endpoint: `/api/v1/tag` })
```
--
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] github-actions[bot] commented on pull request #20876: feat: Frontend tagging
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20876:
URL: https://github.com/apache/superset/pull/20876#issuecomment-1344731335
@lyndsiWilliams Ephemeral environment spinning up at http://54.200.108.246:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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] yousoph commented on pull request #20876: feat: Frontend tagging
Posted by "yousoph (via GitHub)" <gi...@apache.org>.
yousoph commented on PR #20876:
URL: https://github.com/apache/superset/pull/20876#issuecomment-1410857730
/testenv up FEATURE_TAGGING_SYSTEM=true
--
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] hughhhh commented on a diff in pull request #20876: feat: Frontend tagging
Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh commented on code in PR #20876:
URL: https://github.com/apache/superset/pull/20876#discussion_r1110230166
##########
superset/tags/api.py:
##########
@@ -0,0 +1,386 @@
+# 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 logging
+from typing import Any
+
+from flask import request, Response
+from flask_appbuilder.api import expose, protect, rison, safe
+from flask_appbuilder.models.sqla.interface import SQLAInterface
+
+from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod
+from superset.extensions import event_logger
+from superset.tags.commands.create import CreateCustomTagCommand
+from superset.tags.commands.delete import DeleteTaggedObjectCommand, DeleteTagsCommand
+from superset.tags.commands.exceptions import (
+ TagCreateFailedError,
+ TagDeleteFailedError,
+ TaggedObjectDeleteFailedError,
+ TaggedObjectNotFoundError,
+ TagInvalidError,
+ TagNotFoundError,
+)
+from superset.tags.dao import TagDAO
+from superset.tags.models import ObjectTypes, Tag
+from superset.tags.schemas import (
+ delete_tags_schema,
+ openapi_spec_methods_override,
+ TaggedObjectEntityResponseSchema,
+ TagGetResponseSchema,
+ TagPostSchema,
+)
+from superset.views.base_api import (
+ BaseSupersetModelRestApi,
+ RelatedFieldFilter,
+ statsd_metrics,
+)
+from superset.views.filters import BaseFilterRelatedUsers, FilterRelatedOwners
+
+logger = logging.getLogger(__name__)
+
+
+class TagRestApi(BaseSupersetModelRestApi):
+ datamodel = SQLAInterface(Tag)
+ include_route_methods = RouteMethod.REST_MODEL_VIEW_CRUD_SET | {
+ RouteMethod.RELATED,
+ "bulk_delete",
+ "get_objects",
+ "get_all_objects",
+ "add_objects",
+ "delete_object",
+ }
+
+ resource_name = "tag"
Review Comment:
can we change this to be plural `tags`
--
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] cccs-RyanK commented on a diff in pull request #20876: feat: Frontend tagging
Posted by GitBox <gi...@apache.org>.
cccs-RyanK commented on code in PR #20876:
URL: https://github.com/apache/superset/pull/20876#discussion_r1047392206
##########
superset-frontend/src/views/CRUD/tags/TagList.tsx:
##########
@@ -0,0 +1,332 @@
+/**
+ * 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 { t } from '@superset-ui/core';
+import React, { useMemo, useCallback } from 'react';
+import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags';
+import {
+ createFetchRelated,
+ createErrorHandler,
+ Actions,
+} from 'src/views/CRUD/utils';
+import { useListViewResource } from 'src/views/CRUD/hooks';
+import ConfirmStatusChange from 'src/components/ConfirmStatusChange';
+import SubMenu, { SubMenuProps } from 'src/views/components/SubMenu';
+import ListView, {
+ ListViewProps,
+ Filters,
+ FilterOperator,
+} from 'src/components/ListView';
+import { dangerouslyGetItemDoNotUse } from 'src/utils/localStorageHelpers';
+import withToasts from 'src/components/MessageToasts/withToasts';
+import Icons from 'src/components/Icons';
+import { Tooltip } from 'src/components/Tooltip';
+import FacePile from 'src/components/FacePile';
+import { Link } from 'react-router-dom';
+import { deleteTags } from 'src/tags';
+import { Tag as AntdTag } from 'antd';
+import { Tag } from '../types';
+import TagCard from './TagCard';
+
+const PAGE_SIZE = 25;
+
+interface TagListProps {
+ addDangerToast: (msg: string) => void;
+ addSuccessToast: (msg: string) => void;
+ user: {
+ userId: string | number;
+ firstName: string;
+ lastName: string;
+ };
+}
+
+function TagList(props: TagListProps) {
+ const {
+ addDangerToast,
+ addSuccessToast,
+ user: { userId },
+ } = props;
+
+ const {
+ state: {
+ loading,
+ resourceCount: tagCount,
+ resourceCollection: tags,
+ bulkSelectEnabled,
+ },
+ hasPerm,
+ fetchData,
+ toggleBulkSelect,
+ refreshData,
+ } = useListViewResource<Tag>('tag', t('tag'), addDangerToast);
+
+ // TODO: Fix usage of localStorage keying on the user id
+ const userKey = dangerouslyGetItemDoNotUse(userId?.toString(), null);
+
+ const canDelete = hasPerm('can_write');
+
+ const initialSort = [{ id: 'changed_on_delta_humanized', desc: true }];
+
+ function handleTagsDelete(
+ tags: Tag[],
+ callback: (text: string) => void,
+ error: (text: string) => void,
+ ) {
+ // TODO what permissions need to be checked here?
Review Comment:
Yes this is one of the tasks that I am planning to address in a (hopefully near) future 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] github-actions[bot] commented on pull request #20876: feat: Frontend tagging
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20876:
URL: https://github.com/apache/superset/pull/20876#issuecomment-1347350723
@lyndsiWilliams Ephemeral environment spinning up at http://54.190.151.20:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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] rusackas commented on pull request #20876: feature: Frontend tagging
Posted by GitBox <gi...@apache.org>.
rusackas commented on PR #20876:
URL: https://github.com/apache/superset/pull/20876#issuecomment-1198413458
@kasiazjc @jess-dillard FYI!
--
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] jess-dillard commented on pull request #20876: feature: Frontend tagging
Posted by GitBox <gi...@apache.org>.
jess-dillard commented on PR #20876:
URL: https://github.com/apache/superset/pull/20876#issuecomment-1198458819
Hi all, thanks for the great work on this! I have a few UX/UI suggestions that would better align with Superset's current patterns.
**Edit properties modal**
Instead of putting the applied tags to the right of the tag selection field, I would use the multi-select component and keep the tags inside the bounds of the field (like the multi-select filter field in dashboards):
<img width="250" alt="Screen Shot 2022-07-28 at 10 49 11 AM" src="https://user-images.githubusercontent.com/90715511/181603984-0c910a36-fbcc-4024-a976-59a57c953984.png">
**Tags view**
I would actually create two views here to properly utilize and manage tags.
1. Tags CRUD
A Tags page, accessible from the global settings menu, that uses the existing CRUD template to display all tags. Columns in the table could include how many entities are tagged, who created it, etc. This is where the user would manage (edit/add/delete) tags.
2. "All entities" view
To satisfy the need to see all entities with a certain tag applied, I would create another CRUD view that combines dashboards/charts/queries and allows the user to filter by tags. This could be accessible from the main navigation.
--
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] rusackas commented on a diff in pull request #20876: feat: Frontend tagging
Posted by GitBox <gi...@apache.org>.
rusackas commented on code in PR #20876:
URL: https://github.com/apache/superset/pull/20876#discussion_r1053678633
##########
tests/integration_tests/tags/__init__.py:
##########
@@ -0,0 +1,16 @@
+# 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.
Review Comment:
Is this one missing intended tests, or just a vestigial file?
--
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] hughhhh commented on a diff in pull request #20876: feat: Frontend tagging
Posted by GitBox <gi...@apache.org>.
hughhhh commented on code in PR #20876:
URL: https://github.com/apache/superset/pull/20876#discussion_r1062759326
##########
superset/tags/dao.py:
##########
@@ -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 logging
+from typing import Any, Dict
+
+from superset.dao.base import BaseDAO
+from superset.extensions import db
+from superset.tags.models import ObjectTypes, Tag, TaggedObject, TagTypes
+
+logger = logging.getLogger(__name__)
+
+
+class TagDAO(BaseDAO):
+ model_cls = Tag
+ # base_filter = TagAccessFilter
+
+ @staticmethod
+ def create_tagged_objects(
+ object_type: ObjectTypes, object_id: int, properties: Dict[str, Any]
+ ) -> None:
+ tag_names = properties["tags"]
+
+ tagged_objects = []
+ for name in tag_names:
Review Comment:
can we write a test to make sure this logic is solid
--
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] cccs-RyanK closed pull request #20876: feat: Frontend tagging
Posted by GitBox <gi...@apache.org>.
cccs-RyanK closed pull request #20876: feat: Frontend tagging
URL: https://github.com/apache/superset/pull/20876
--
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] lyndsiWilliams commented on pull request #20876: feat: Frontend tagging
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on PR #20876:
URL: https://github.com/apache/superset/pull/20876#issuecomment-1347344258
> > /testenv up TAGGING_SYSTEM=true
>
> I think this needs to be `FEATURE_TAGGING_SYSTEM` for test envs
Whoops! I was close haha 😅
--
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] kasiazjc commented on pull request #20876: feature: Frontend tagging
Posted by GitBox <gi...@apache.org>.
kasiazjc commented on PR #20876:
URL: https://github.com/apache/superset/pull/20876#issuecomment-1200842814
> > > Hi all, thanks for the great work on this! I have a few UX/UI suggestions that would better align with Superset's current patterns.
> > > **Edit properties modal** Instead of putting the applied tags to the right of the tag selection field, I would use the multi-select component and keep the tags inside the bounds of the field (like the multi-select filter field in dashboards): <img alt="Screen Shot 2022-07-28 at 10 49 11 AM" width="250" src="https://user-images.githubusercontent.com/90715511/181603984-0c910a36-fbcc-4024-a976-59a57c953984.png">
> > > **Tags view** I would actually create two views here to properly utilize and manage tags.
> > >
> > > 1. Tags CRUD
> > > A Tags page, accessible from the global settings menu, that uses the existing CRUD template to display all tags. Columns in the table could include how many entities are tagged, who created it, etc. This is where the user would manage (edit/add/delete) tags.
> > > 2. "All entities" view
> > > To satisfy the need to see all entities with a certain tag applied, I would create another CRUD view that combines dashboards/charts/queries and allows the user to filter by tags. This could be accessible from the main navigation.
> >
> >
> > Thanks for working on this! Have a few additional comments.
> > **Crud view** I think the field in the filter bar should be a multi select, not a search. Search does not show all of the possible tags and I think it would be hard to remember all of them to choose the right one.
> > Our multiple select currently expands in height when multiple values are selected before truncating, so it would have to have fixed height and truncate earlier.
> > **Object pages** Do tags have to be displayed in the header? I guess they will be mostly used for searching/grouping things.
> > ```
> > 1. It clutters the header + the question on what is more important comes up. Let's say you have a long name of the chart and 10 tags. I would say that title is more important in this case. How tags would be truncated?
> >
> > 2. We are adding something called metadata bar to the header, which will display the most important info about the object. We could add a section "tags" which would display all of the tags on hover for example. Below screenshot on how the metadata bar will look like.
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > <img alt="image" width="1306" src="https://user-images.githubusercontent.com/36897697/181609940-297df6ca-16dc-4765-8bfa-7bb83541acd2.png">
>
> Thank you for the quick feedback @kasiazjc and @jess-dillard ! I agree I think tags are mostly useful in the CRUD views where objects can be grouped/filtered by using tags, and not so much in the headers like you said.
>
> For your question on truncating tags, the length of an individual tag's text is limited to 20 characters right now. In terms of truncating the list of tags, there is a maxTags attribute for the TagsList component that can be used to limit the number of tags shown by default. Currently, you can click on the last tag that shows the number of truncated tags to toggle between showing the full list of tags and the truncated list. I really like the idea you mentioned of hovering over the component, and it showing all the tags in a small popup view, but I didn't end up getting around to implementing it in this PR. Here is an example with a dashboard with a long title and too many tags:
>
>  
>
> Perhaps it would be best to remove the tags from the object headers for now and then consider adding them to the metadata bar when that is introduced? Is there any timeline on when the metadata bar will be pushed?
Thanks for quick response too 🙏
We will be taking up metadata bar this sprint. So for now, let's remove the tags from the header and we'll try to fit it in the metadata bar :)
--
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] github-actions[bot] commented on pull request #20876: feat: Frontend tagging
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20876:
URL: https://github.com/apache/superset/pull/20876#issuecomment-1397453447
Ephemeral environment shutdown and build artifacts deleted.
--
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] rusackas commented on pull request #20876: feat: Frontend tagging
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #20876:
URL: https://github.com/apache/superset/pull/20876#issuecomment-1401085705
@hughhhh / @lyndsiWilliams / @eschutho looks like perhaps this is good to go, perhaps pending @hughhhh 's backend code approval?
--
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] villebro commented on pull request #20876: feat: Frontend tagging
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #20876:
URL: https://github.com/apache/superset/pull/20876#issuecomment-1427105976
@cccs-RyanK I believe this needs a rebase
--
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] hughhhh commented on a diff in pull request #20876: feat: Frontend tagging
Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh commented on code in PR #20876:
URL: https://github.com/apache/superset/pull/20876#discussion_r1107669240
##########
superset-frontend/src/tags.ts:
##########
@@ -0,0 +1,187 @@
+/**
+ * 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 { JsonObject, SupersetClient } from '@superset-ui/core';
+import rison from 'rison';
+import Tag from 'src/types/TagType';
+
+export const OBJECT_TYPES_VALUES = Object.freeze([
+ 'dashboard',
+ 'chart',
+ 'saved_query',
+]);
+
+export const OBJECT_TYPES = Object.freeze({
+ DASHBOARD: 'dashboard',
+ CHART: 'chart',
+ QUERY: 'saved_query',
+});
+
+const OBJECT_TYPE_ID_MAP = {
+ saved_query: 1,
+ chart: 2,
+ dashboard: 3,
+};
+
+const map_object_type_to_id = (objectType: string) => {
+ if (!OBJECT_TYPES_VALUES.includes(objectType)) {
+ const msg = `objectType ${objectType} is invalid`;
+ throw new Error(msg);
+ }
+ return OBJECT_TYPE_ID_MAP[objectType];
+};
+
+export function fetchAllTags(
+ callback: (json: JsonObject) => void,
+ error: (response: Response) => void,
+) {
+ const url = `/api/v1/tag`;
+ SupersetClient.get({ endpoint: url })
Review Comment:
nit: just put the url inside the function
```suggestion
SupersetClient.get({ endpoint: `/api/v1/tag` })
```
--
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] cccs-RyanK commented on pull request #20876: feature: Frontend tagging
Posted by GitBox <gi...@apache.org>.
cccs-RyanK commented on PR #20876:
URL: https://github.com/apache/superset/pull/20876#issuecomment-1199848057
> > Hi all, thanks for the great work on this! I have a few UX/UI suggestions that would better align with Superset's current patterns.
> > **Edit properties modal** Instead of putting the applied tags to the right of the tag selection field, I would use the multi-select component and keep the tags inside the bounds of the field (like the multi-select filter field in dashboards): <img alt="Screen Shot 2022-07-28 at 10 49 11 AM" width="250" src="https://user-images.githubusercontent.com/90715511/181603984-0c910a36-fbcc-4024-a976-59a57c953984.png">
> > **Tags view** I would actually create two views here to properly utilize and manage tags.
> >
> > 1. Tags CRUD
> > A Tags page, accessible from the global settings menu, that uses the existing CRUD template to display all tags. Columns in the table could include how many entities are tagged, who created it, etc. This is where the user would manage (edit/add/delete) tags.
> > 2. "All entities" view
> > To satisfy the need to see all entities with a certain tag applied, I would create another CRUD view that combines dashboards/charts/queries and allows the user to filter by tags. This could be accessible from the main navigation.
>
> Thanks for working on this! Have a few additional comments.
>
> **Crud view** I think the field in the filter bar should be a multi select, not a search. Search does not show all of the possible tags and I think it would be hard to remember all of them to choose the right one.
>
> Our multiple select currently expands in height when multiple values are selected before truncating, so it would have to have fixed height and truncate earlier.
>
> **Object pages** Do tags have to be displayed in the header? I guess they will be mostly used for searching/grouping things.
>
> 1. It clutters the header + the question on what is more important comes up. Let's say you have a long name of the chart and 10 tags. I would say that title is more important in this case. How tags would be truncated?
>
> 2. We are adding something called metadata bar to the header, which will display the most important info about the object. We could add a section "tags" which would display all of the tags on hover for example. Below screenshot on how the metadata bar will look like.
>
>
> <img alt="image" width="1306" src="https://user-images.githubusercontent.com/36897697/181609940-297df6ca-16dc-4765-8bfa-7bb83541acd2.png">
Thank you for the quick feedback @kasiazjc and @jess-dillard ! I agree I think tags are mostly useful in the CRUD views where objects can be grouped/filtered by using tags, and not so much in the headers like you said.
For your question on truncating tags, the length of an individual tag's text is limited to 20 characters right now. In terms of truncating the list of tags, there is a maxTags attribute for the TagsList component that can be used to limit the number of tags shown by default. Currently, you can click on the last tag that shows the number of truncated tags to toggle between showing the full list of tags and the truncated list. I really like the idea you mentioned of hovering over the component, and it showing all the tags in a small popup view, but I didn't end up getting around to implementing it in this PR. Here is an example with a dashboard with a long title and too many tags:
Perhaps it would be best to remove the tags from the object headers for now and then consider adding them to the metadata bar when that is introduced? Is there any timeline on when the metadata bar will be pushed?
--
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] lyndsiWilliams commented on a diff in pull request #20876: feature: Frontend tagging
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #20876:
URL: https://github.com/apache/superset/pull/20876#discussion_r947216424
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/package.json:
##########
@@ -48,6 +48,7 @@
"react-dom": "^16.13.1",
"react-map-gl": "^4.0.10",
"mapbox-gl": "*"
+
Review Comment:
Nit: remove this space.
##########
superset-frontend/src/components/ObjectTags/ObjectTags.css:
##########
@@ -0,0 +1,149 @@
+/**
+ * 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.
+ */
+.ant-tag {
+ color: black;
+}
+
+.react-tags {
+ position: relative;
+ display: inline-block;
+ padding: 1px 0 0 1px;
+ margin: 0 10px;
+ border: 0px solid #f5f5f5;
+ border-radius: 1px;
+
+ /* shared font styles */
+ font-size: 12px;
+ line-height: 1.2;
+
+ /* clicking anywhere will focus the input */
+ cursor: text;
+}
+
+.react-tags__selected {
+ display: inline;
+}
+
+.react-tags__selected-tag {
+ display: inline-block;
+ box-sizing: border-box;
+ margin: 0;
+ padding: 6px 8px;
+ border: 0px solid #f5f5f5;
+ border-radius: 2px;
Review Comment:
```suggestion
border-radius: ${({ theme }) => theme.borderRadius}px;
```
Border-radius values should stay within the Superset theme, the current value is 4 as seen [here](https://github.com/apache/superset/blob/master/superset-frontend/packages/superset-ui-core/src/style/index.tsx#L55).
##########
superset-frontend/src/components/ObjectTags/index.tsx:
##########
@@ -0,0 +1,166 @@
+/**
+ * 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, { useEffect, useState } from 'react';
+import { styled, SupersetClient, t } from '@superset-ui/core';
+import Tag from 'src/types/TagType';
+
+import './ObjectTags.css';
+import { TagsList } from 'src/components/Tags';
+import rison from 'rison';
+import { cacheWrapper } from 'src/utils/cacheWrapper';
+import {
+ ClientErrorObject,
+ getClientErrorObject,
+} from 'src/utils/getClientErrorObject';
+import { deleteTag, fetchTags } from 'src/tags';
+
+export interface ObjectTagsProps {
+ objectType: string;
+ objectId: number;
+ includeTypes: boolean;
+ editMode: boolean;
+ maxTags: number | undefined;
+ onChange?: (tags: Tag[]) => void;
+}
+
+const StyledTagsDiv = styled.div`
+ margin-left: ${({ theme }) => theme.gridUnit * 2}px;
+ max-width: 100%;
+ display: -webkit-flex;
+ display: flex;
+ -webkit-flex-direction: row;
+ -webkit-flex-wrap: wrap;
+`;
+
+const localCache = new Map<string, any>();
+
+const cachedSupersetGet = cacheWrapper(
+ SupersetClient.get,
+ localCache,
+ ({ endpoint }) => endpoint || '',
+);
+
+type SelectTagsValue = {
+ value: string | number | undefined;
+ label: string;
+};
+
+export const tagToSelectOption = (
+ item: Tag & { table_name: string },
+): SelectTagsValue => ({
+ value: item.id,
+ label: item.name,
+});
+
+export const loadTags = async (
+ search: string,
+ page: number,
+ pageSize: number,
+) => {
+ const searchColumn = 'name';
+ const query = rison.encode({
+ filters: [{ col: searchColumn, opr: 'ct', value: search }],
+ page,
+ page_size: pageSize,
+ order_column: searchColumn,
+ order_direction: 'asc',
+ });
+
+ const getErrorMessage = ({ error, message }: ClientErrorObject) => {
+ let errorText = message || error || t('An error has occurred');
+ if (message === 'Forbidden') {
+ errorText = t('You do not have permission to edit this dashboard');
+ }
+ return errorText;
+ };
+
+ return cachedSupersetGet({
+ endpoint: `/api/v1/tag/?q=${query}`,
+ // endpoint: `/api/v1/tags/?q=${query}`,
Review Comment:
Nit: Remove this comment
##########
superset-frontend/src/components/ObjectTags/ObjectTags.css:
##########
@@ -0,0 +1,149 @@
+/**
+ * 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.
+ */
+.ant-tag {
+ color: black;
Review Comment:
```suggestion
color: ${({ theme }) => theme.colors.grayscale.dark2};
```
Color values should stay within the Superset theme
##########
superset-frontend/src/components/ObjectTags/ObjectTags.css:
##########
@@ -0,0 +1,149 @@
+/**
+ * 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.
+ */
+.ant-tag {
+ color: black;
+}
+
+.react-tags {
+ position: relative;
+ display: inline-block;
+ padding: 1px 0 0 1px;
+ margin: 0 10px;
+ border: 0px solid #f5f5f5;
+ border-radius: 1px;
+
+ /* shared font styles */
+ font-size: 12px;
+ line-height: 1.2;
+
+ /* clicking anywhere will focus the input */
+ cursor: text;
+}
+
+.react-tags__selected {
+ display: inline;
+}
+
+.react-tags__selected-tag {
+ display: inline-block;
+ box-sizing: border-box;
+ margin: 0;
+ padding: 6px 8px;
Review Comment:
```suggestion
padding: ${({ theme }) => theme.gridUnit * 1.5}px
${({ theme }) => theme.gridUnit * 2}px;
```
Unit values should stay within the Superset theme
##########
superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.tsx:
##########
@@ -362,6 +364,20 @@ function SavedQueryList({
accessor: 'changed_on_delta_humanized',
size: 'xl',
},
+ {
+ Cell: ({
+ row: {
+ original: { tags = [] },
+ },
+ }: any) => (
Review Comment:
Nit: Describe `any`
##########
superset-frontend/src/components/ObjectTags/ObjectTags.css:
##########
@@ -0,0 +1,149 @@
+/**
+ * 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.
+ */
+.ant-tag {
+ color: black;
+}
+
+.react-tags {
+ position: relative;
+ display: inline-block;
+ padding: 1px 0 0 1px;
+ margin: 0 10px;
+ border: 0px solid #f5f5f5;
+ border-radius: 1px;
+
+ /* shared font styles */
+ font-size: 12px;
+ line-height: 1.2;
+
+ /* clicking anywhere will focus the input */
+ cursor: text;
+}
+
+.react-tags__selected {
+ display: inline;
+}
+
+.react-tags__selected-tag {
+ display: inline-block;
+ box-sizing: border-box;
+ margin: 0;
+ padding: 6px 8px;
+ border: 0px solid #f5f5f5;
+ border-radius: 2px;
+ background: #f1f1f1;
Review Comment:
Color values should stay within the Superset theme. The available color values can be found [here](https://github.com/apache/superset/blob/master/superset-frontend/packages/superset-ui-core/src/style/index.tsx#L56-L127).
##########
superset-frontend/src/components/ObjectTags/ObjectTags.css:
##########
@@ -0,0 +1,149 @@
+/**
+ * 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.
+ */
+.ant-tag {
+ color: black;
+}
+
+.react-tags {
+ position: relative;
+ display: inline-block;
+ padding: 1px 0 0 1px;
+ margin: 0 10px;
+ border: 0px solid #f5f5f5;
+ border-radius: 1px;
+
+ /* shared font styles */
+ font-size: 12px;
+ line-height: 1.2;
+
+ /* clicking anywhere will focus the input */
+ cursor: text;
+}
+
+.react-tags__selected {
+ display: inline;
+}
+
+.react-tags__selected-tag {
+ display: inline-block;
+ box-sizing: border-box;
+ margin: 0;
+ padding: 6px 8px;
+ border: 0px solid #f5f5f5;
+ border-radius: 2px;
+ background: #f1f1f1;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search {
+ display: inline-block;
+
+ /* new tag border layout */
+ border: 1px dashed #d9d9d9;
+
+ /* match tag layout */
+ line-height: 20px;
+ margin-bottom: 0;
+ padding: 0 7px;
+
+ /* prevent autoresize overflowing the container */
+ max-width: 100%;
+}
+
+.react-tags__search:focus-within {
+ border: 1px solid #000000;
+}
+
+@media screen and (min-width: 30em) {
+ .react-tags__search {
+ /* this will become the offsetParent for suggestions */
+ position: relative;
+ }
+}
+
+.react-tags__search input {
+ max-width: 150%;
+
+ /* remove styles and layout from this element */
+ margin: 0;
+ margin-left: 0;
+ padding: 0;
+ border: 0;
+ outline: none;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search input::-ms-clear {
+ display: none;
+}
+
+.react-tags__suggestions {
+ position: absolute;
+ top: 100%;
+ left: 0;
+ width: 100%;
+ z-index: 9999;
+}
+
+@media screen and (min-width: 30em) {
+ .react-tags__suggestions {
+ width: 240px;
+ }
+}
+
+.react-tags__suggestions ul {
+ margin: 4px -1px;
+ padding: 0;
+ list-style: none;
+ background: white;
+ border: 1px solid #d1d1d1;
Review Comment:
Color values should stay within the Superset theme. The available color values can be found [here](https://github.com/apache/superset/blob/master/superset-frontend/packages/superset-ui-core/src/style/index.tsx#L56-L127).
##########
superset-frontend/src/components/ObjectTags/ObjectTags.css:
##########
@@ -0,0 +1,149 @@
+/**
+ * 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.
+ */
+.ant-tag {
+ color: black;
+}
+
+.react-tags {
+ position: relative;
+ display: inline-block;
+ padding: 1px 0 0 1px;
+ margin: 0 10px;
+ border: 0px solid #f5f5f5;
+ border-radius: 1px;
+
+ /* shared font styles */
+ font-size: 12px;
+ line-height: 1.2;
+
+ /* clicking anywhere will focus the input */
+ cursor: text;
+}
+
+.react-tags__selected {
+ display: inline;
+}
+
+.react-tags__selected-tag {
+ display: inline-block;
+ box-sizing: border-box;
+ margin: 0;
+ padding: 6px 8px;
+ border: 0px solid #f5f5f5;
Review Comment:
I see that this border is being set to `0px` but also setting a color. Is the intent to hide the border or set it as a different color?
##########
superset-frontend/src/components/ObjectTags/ObjectTags.css:
##########
@@ -0,0 +1,149 @@
+/**
+ * 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.
+ */
+.ant-tag {
+ color: black;
+}
+
+.react-tags {
+ position: relative;
+ display: inline-block;
+ padding: 1px 0 0 1px;
+ margin: 0 10px;
Review Comment:
```suggestion
margin: 0 ${({ theme }) => theme.gridUnit * 2.5}px;
```
Unit values should stay within the Superset theme
##########
superset-frontend/src/components/ObjectTags/ObjectTags.css:
##########
@@ -0,0 +1,149 @@
+/**
+ * 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.
+ */
+.ant-tag {
+ color: black;
+}
+
+.react-tags {
+ position: relative;
+ display: inline-block;
+ padding: 1px 0 0 1px;
+ margin: 0 10px;
+ border: 0px solid #f5f5f5;
+ border-radius: 1px;
+
+ /* shared font styles */
+ font-size: 12px;
Review Comment:
```suggestion
font-size: ${({ theme }) => theme.gridUnit * 3}px;
```
Unit values should stay within the Superset theme
##########
superset-frontend/src/components/ObjectTags/ObjectTags.css:
##########
@@ -0,0 +1,149 @@
+/**
+ * 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.
+ */
+.ant-tag {
+ color: black;
+}
+
+.react-tags {
+ position: relative;
+ display: inline-block;
+ padding: 1px 0 0 1px;
+ margin: 0 10px;
+ border: 0px solid #f5f5f5;
+ border-radius: 1px;
+
+ /* shared font styles */
+ font-size: 12px;
+ line-height: 1.2;
+
+ /* clicking anywhere will focus the input */
+ cursor: text;
+}
+
+.react-tags__selected {
+ display: inline;
+}
+
+.react-tags__selected-tag {
+ display: inline-block;
+ box-sizing: border-box;
+ margin: 0;
+ padding: 6px 8px;
+ border: 0px solid #f5f5f5;
+ border-radius: 2px;
+ background: #f1f1f1;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search {
+ display: inline-block;
+
+ /* new tag border layout */
+ border: 1px dashed #d9d9d9;
+
+ /* match tag layout */
+ line-height: 20px;
+ margin-bottom: 0;
+ padding: 0 7px;
+
+ /* prevent autoresize overflowing the container */
+ max-width: 100%;
+}
+
+.react-tags__search:focus-within {
+ border: 1px solid #000000;
+}
+
+@media screen and (min-width: 30em) {
+ .react-tags__search {
+ /* this will become the offsetParent for suggestions */
+ position: relative;
+ }
+}
+
+.react-tags__search input {
+ max-width: 150%;
+
+ /* remove styles and layout from this element */
+ margin: 0;
+ margin-left: 0;
+ padding: 0;
+ border: 0;
+ outline: none;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search input::-ms-clear {
+ display: none;
+}
+
+.react-tags__suggestions {
+ position: absolute;
+ top: 100%;
+ left: 0;
+ width: 100%;
+ z-index: 9999;
+}
+
+@media screen and (min-width: 30em) {
+ .react-tags__suggestions {
+ width: 240px;
+ }
+}
+
+.react-tags__suggestions ul {
+ margin: 4px -1px;
+ padding: 0;
+ list-style: none;
+ background: white;
+ border: 1px solid #d1d1d1;
+ border-radius: 2px;
+ box-shadow: 0 2px 6px rgba(0, 0, 0, 0.2);
+}
+
+.react-tags__suggestions li {
+ border-bottom: 1px solid #ddd;
+ padding: 6px 8px;
Review Comment:
```suggestion
padding: ${({ theme }) => theme.gridUnit * 1.5}px
${({ theme }) => theme.gridUnit * 2}px;
```
Unit values should stay within the Superset theme
##########
superset-frontend/src/components/ObjectTags/ObjectTags.css:
##########
@@ -0,0 +1,149 @@
+/**
+ * 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.
+ */
+.ant-tag {
+ color: black;
+}
+
+.react-tags {
+ position: relative;
+ display: inline-block;
+ padding: 1px 0 0 1px;
+ margin: 0 10px;
+ border: 0px solid #f5f5f5;
+ border-radius: 1px;
+
+ /* shared font styles */
+ font-size: 12px;
+ line-height: 1.2;
+
+ /* clicking anywhere will focus the input */
+ cursor: text;
+}
+
+.react-tags__selected {
+ display: inline;
+}
+
+.react-tags__selected-tag {
+ display: inline-block;
+ box-sizing: border-box;
+ margin: 0;
+ padding: 6px 8px;
+ border: 0px solid #f5f5f5;
+ border-radius: 2px;
+ background: #f1f1f1;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search {
+ display: inline-block;
+
+ /* new tag border layout */
+ border: 1px dashed #d9d9d9;
Review Comment:
Color values should stay within the Superset theme. The available color values can be found [here](https://github.com/apache/superset/blob/master/superset-frontend/packages/superset-ui-core/src/style/index.tsx#L56-L127).
##########
superset-frontend/src/components/ObjectTags/ObjectTags.css:
##########
@@ -0,0 +1,149 @@
+/**
+ * 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.
+ */
+.ant-tag {
+ color: black;
+}
+
+.react-tags {
+ position: relative;
+ display: inline-block;
+ padding: 1px 0 0 1px;
+ margin: 0 10px;
+ border: 0px solid #f5f5f5;
+ border-radius: 1px;
+
+ /* shared font styles */
+ font-size: 12px;
+ line-height: 1.2;
+
+ /* clicking anywhere will focus the input */
+ cursor: text;
+}
+
+.react-tags__selected {
+ display: inline;
+}
+
+.react-tags__selected-tag {
+ display: inline-block;
+ box-sizing: border-box;
+ margin: 0;
+ padding: 6px 8px;
+ border: 0px solid #f5f5f5;
+ border-radius: 2px;
+ background: #f1f1f1;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search {
+ display: inline-block;
+
+ /* new tag border layout */
+ border: 1px dashed #d9d9d9;
+
+ /* match tag layout */
+ line-height: 20px;
+ margin-bottom: 0;
+ padding: 0 7px;
+
+ /* prevent autoresize overflowing the container */
+ max-width: 100%;
+}
+
+.react-tags__search:focus-within {
+ border: 1px solid #000000;
+}
+
+@media screen and (min-width: 30em) {
+ .react-tags__search {
+ /* this will become the offsetParent for suggestions */
+ position: relative;
+ }
+}
+
+.react-tags__search input {
+ max-width: 150%;
+
+ /* remove styles and layout from this element */
+ margin: 0;
+ margin-left: 0;
+ padding: 0;
+ border: 0;
+ outline: none;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search input::-ms-clear {
+ display: none;
+}
+
+.react-tags__suggestions {
+ position: absolute;
+ top: 100%;
+ left: 0;
+ width: 100%;
+ z-index: 9999;
+}
+
+@media screen and (min-width: 30em) {
+ .react-tags__suggestions {
+ width: 240px;
+ }
+}
+
+.react-tags__suggestions ul {
+ margin: 4px -1px;
+ padding: 0;
+ list-style: none;
+ background: white;
+ border: 1px solid #d1d1d1;
+ border-radius: 2px;
+ box-shadow: 0 2px 6px rgba(0, 0, 0, 0.2);
+}
+
+.react-tags__suggestions li {
+ border-bottom: 1px solid #ddd;
+ padding: 6px 8px;
+}
+
+.react-tags__suggestions li mark {
+ text-decoration: underline;
+ background: none;
+ font-weight: 600;
+}
+
+.react-tags__suggestions li:hover {
+ cursor: pointer;
+ background: #eee;
+}
+
+.react-tags__suggestions li.is-active {
+ background: #b7cfe0;
Review Comment:
Color values should stay within the Superset theme. The available color values can be found [here](https://github.com/apache/superset/blob/master/superset-frontend/packages/superset-ui-core/src/style/index.tsx#L56-L127).
##########
superset-frontend/src/components/ObjectTags/ObjectTags.css:
##########
@@ -0,0 +1,149 @@
+/**
+ * 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.
+ */
+.ant-tag {
+ color: black;
+}
+
+.react-tags {
+ position: relative;
+ display: inline-block;
+ padding: 1px 0 0 1px;
+ margin: 0 10px;
+ border: 0px solid #f5f5f5;
+ border-radius: 1px;
+
+ /* shared font styles */
+ font-size: 12px;
+ line-height: 1.2;
+
+ /* clicking anywhere will focus the input */
+ cursor: text;
+}
+
+.react-tags__selected {
+ display: inline;
+}
+
+.react-tags__selected-tag {
+ display: inline-block;
+ box-sizing: border-box;
+ margin: 0;
+ padding: 6px 8px;
+ border: 0px solid #f5f5f5;
+ border-radius: 2px;
+ background: #f1f1f1;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search {
+ display: inline-block;
+
+ /* new tag border layout */
+ border: 1px dashed #d9d9d9;
+
+ /* match tag layout */
+ line-height: 20px;
Review Comment:
```suggestion
line-height: ${({ theme }) => theme.gridUnit * 5}px;
```
Unit values should stay within the Superset theme
##########
superset-frontend/src/components/ObjectTags/ObjectTags.css:
##########
@@ -0,0 +1,149 @@
+/**
+ * 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.
+ */
+.ant-tag {
+ color: black;
+}
+
+.react-tags {
+ position: relative;
+ display: inline-block;
+ padding: 1px 0 0 1px;
+ margin: 0 10px;
+ border: 0px solid #f5f5f5;
+ border-radius: 1px;
+
+ /* shared font styles */
+ font-size: 12px;
+ line-height: 1.2;
+
+ /* clicking anywhere will focus the input */
+ cursor: text;
+}
+
+.react-tags__selected {
+ display: inline;
+}
+
+.react-tags__selected-tag {
+ display: inline-block;
+ box-sizing: border-box;
+ margin: 0;
+ padding: 6px 8px;
+ border: 0px solid #f5f5f5;
+ border-radius: 2px;
+ background: #f1f1f1;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search {
+ display: inline-block;
+
+ /* new tag border layout */
+ border: 1px dashed #d9d9d9;
+
+ /* match tag layout */
+ line-height: 20px;
+ margin-bottom: 0;
+ padding: 0 7px;
Review Comment:
```suggestion
padding: 0 ${({ theme }) => theme.gridUnit * 1.75}px;
```
Unit values should stay within the Superset theme
##########
superset-frontend/src/components/Tags/TagsList.tsx:
##########
@@ -0,0 +1,111 @@
+/**
+ * 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, { useMemo, useState } from 'react';
+import { styled } from '@superset-ui/core';
+import TagType from 'src/types/TagType';
+import Tag from './Tag';
+
+export type TagsListProps = {
+ tags: TagType[];
+ editable?: boolean;
+ /**
+ * OnDelete:
+ * Only applies when editable is true
+ * Callback for when a tag is deleted
+ */
+ onDelete?: ((index: number) => void) | undefined;
+ maxTags?: number | undefined;
+};
+
+const TagsDiv = styled.div`
+ max-width: 100%;
+ display: -webkit-flex;
+ display: flex;
+ -webkit-flex-direction: row;
+ -webkit-flex-wrap: wrap;
Review Comment:
```suggestion
display: flex;
flex-direction: row;
flex-wrap: wrap;
```
Emotion uses the [Stylis CSS Preprocessor](https://github.com/thysultan/stylis), which handles these prefixes on runtime. See [this thread](https://github.com/emotion-js/emotion/issues/1523#issuecomment-535865527) for more info
##########
superset-frontend/src/components/ObjectTags/ObjectTags.css:
##########
@@ -0,0 +1,149 @@
+/**
+ * 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.
+ */
+.ant-tag {
+ color: black;
+}
+
+.react-tags {
+ position: relative;
+ display: inline-block;
+ padding: 1px 0 0 1px;
+ margin: 0 10px;
+ border: 0px solid #f5f5f5;
+ border-radius: 1px;
+
+ /* shared font styles */
+ font-size: 12px;
+ line-height: 1.2;
+
+ /* clicking anywhere will focus the input */
+ cursor: text;
+}
+
+.react-tags__selected {
+ display: inline;
+}
+
+.react-tags__selected-tag {
+ display: inline-block;
+ box-sizing: border-box;
+ margin: 0;
+ padding: 6px 8px;
+ border: 0px solid #f5f5f5;
+ border-radius: 2px;
+ background: #f1f1f1;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search {
+ display: inline-block;
+
+ /* new tag border layout */
+ border: 1px dashed #d9d9d9;
+
+ /* match tag layout */
+ line-height: 20px;
+ margin-bottom: 0;
+ padding: 0 7px;
+
+ /* prevent autoresize overflowing the container */
+ max-width: 100%;
+}
+
+.react-tags__search:focus-within {
+ border: 1px solid #000000;
+}
+
+@media screen and (min-width: 30em) {
+ .react-tags__search {
+ /* this will become the offsetParent for suggestions */
+ position: relative;
+ }
+}
+
+.react-tags__search input {
+ max-width: 150%;
+
+ /* remove styles and layout from this element */
+ margin: 0;
+ margin-left: 0;
+ padding: 0;
+ border: 0;
+ outline: none;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search input::-ms-clear {
+ display: none;
+}
+
+.react-tags__suggestions {
+ position: absolute;
+ top: 100%;
+ left: 0;
+ width: 100%;
+ z-index: 9999;
+}
+
+@media screen and (min-width: 30em) {
+ .react-tags__suggestions {
+ width: 240px;
+ }
+}
+
+.react-tags__suggestions ul {
+ margin: 4px -1px;
+ padding: 0;
+ list-style: none;
+ background: white;
+ border: 1px solid #d1d1d1;
+ border-radius: 2px;
+ box-shadow: 0 2px 6px rgba(0, 0, 0, 0.2);
+}
+
+.react-tags__suggestions li {
+ border-bottom: 1px solid #ddd;
Review Comment:
Color values should stay within the Superset theme. The available color values can be found [here](https://github.com/apache/superset/blob/master/superset-frontend/packages/superset-ui-core/src/style/index.tsx#L56-L127).
##########
superset-frontend/src/components/ObjectTags/ObjectTags.css:
##########
@@ -0,0 +1,149 @@
+/**
+ * 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.
+ */
+.ant-tag {
+ color: black;
+}
+
+.react-tags {
+ position: relative;
+ display: inline-block;
+ padding: 1px 0 0 1px;
+ margin: 0 10px;
+ border: 0px solid #f5f5f5;
+ border-radius: 1px;
+
+ /* shared font styles */
+ font-size: 12px;
+ line-height: 1.2;
+
+ /* clicking anywhere will focus the input */
+ cursor: text;
+}
+
+.react-tags__selected {
+ display: inline;
+}
+
+.react-tags__selected-tag {
+ display: inline-block;
+ box-sizing: border-box;
+ margin: 0;
+ padding: 6px 8px;
+ border: 0px solid #f5f5f5;
+ border-radius: 2px;
+ background: #f1f1f1;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search {
+ display: inline-block;
+
+ /* new tag border layout */
+ border: 1px dashed #d9d9d9;
+
+ /* match tag layout */
+ line-height: 20px;
+ margin-bottom: 0;
+ padding: 0 7px;
+
+ /* prevent autoresize overflowing the container */
+ max-width: 100%;
+}
+
+.react-tags__search:focus-within {
+ border: 1px solid #000000;
+}
+
+@media screen and (min-width: 30em) {
+ .react-tags__search {
+ /* this will become the offsetParent for suggestions */
+ position: relative;
+ }
+}
+
+.react-tags__search input {
+ max-width: 150%;
+
+ /* remove styles and layout from this element */
+ margin: 0;
+ margin-left: 0;
+ padding: 0;
+ border: 0;
+ outline: none;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search input::-ms-clear {
+ display: none;
+}
+
+.react-tags__suggestions {
+ position: absolute;
+ top: 100%;
+ left: 0;
+ width: 100%;
+ z-index: 9999;
+}
+
+@media screen and (min-width: 30em) {
+ .react-tags__suggestions {
+ width: 240px;
+ }
+}
+
+.react-tags__suggestions ul {
+ margin: 4px -1px;
+ padding: 0;
+ list-style: none;
+ background: white;
+ border: 1px solid #d1d1d1;
+ border-radius: 2px;
Review Comment:
```suggestion
border-radius: ${({ theme }) => theme.borderRadius}px;
```
Border-radius values should stay within the Superset theme, the current value is 4 as seen [here](https://github.com/apache/superset/blob/master/superset-frontend/packages/superset-ui-core/src/style/index.tsx#L55).
##########
superset-frontend/src/components/ObjectTags/ObjectTags.css:
##########
@@ -0,0 +1,149 @@
+/**
+ * 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.
+ */
+.ant-tag {
+ color: black;
+}
+
+.react-tags {
+ position: relative;
+ display: inline-block;
+ padding: 1px 0 0 1px;
+ margin: 0 10px;
+ border: 0px solid #f5f5f5;
+ border-radius: 1px;
+
+ /* shared font styles */
+ font-size: 12px;
+ line-height: 1.2;
+
+ /* clicking anywhere will focus the input */
+ cursor: text;
+}
+
+.react-tags__selected {
+ display: inline;
+}
+
+.react-tags__selected-tag {
+ display: inline-block;
+ box-sizing: border-box;
+ margin: 0;
+ padding: 6px 8px;
+ border: 0px solid #f5f5f5;
+ border-radius: 2px;
+ background: #f1f1f1;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search {
+ display: inline-block;
+
+ /* new tag border layout */
+ border: 1px dashed #d9d9d9;
+
+ /* match tag layout */
+ line-height: 20px;
+ margin-bottom: 0;
+ padding: 0 7px;
+
+ /* prevent autoresize overflowing the container */
+ max-width: 100%;
+}
+
+.react-tags__search:focus-within {
+ border: 1px solid #000000;
+}
+
+@media screen and (min-width: 30em) {
+ .react-tags__search {
+ /* this will become the offsetParent for suggestions */
+ position: relative;
+ }
+}
+
+.react-tags__search input {
+ max-width: 150%;
+
+ /* remove styles and layout from this element */
+ margin: 0;
+ margin-left: 0;
+ padding: 0;
+ border: 0;
+ outline: none;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search input::-ms-clear {
+ display: none;
+}
+
+.react-tags__suggestions {
+ position: absolute;
+ top: 100%;
+ left: 0;
+ width: 100%;
+ z-index: 9999;
Review Comment:
```suggestion
z-index: ${({ theme }) => theme.zIndex.max};
```
Z-index values should stay within the Superset theme.
Our theme sets the max z-index as 3000. Will that work for this, or does it need to be higher?
##########
superset-frontend/src/components/ObjectTags/ObjectTags.css:
##########
@@ -0,0 +1,149 @@
+/**
+ * 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.
+ */
+.ant-tag {
+ color: black;
+}
+
+.react-tags {
+ position: relative;
+ display: inline-block;
+ padding: 1px 0 0 1px;
+ margin: 0 10px;
+ border: 0px solid #f5f5f5;
+ border-radius: 1px;
+
+ /* shared font styles */
+ font-size: 12px;
+ line-height: 1.2;
+
+ /* clicking anywhere will focus the input */
+ cursor: text;
+}
+
+.react-tags__selected {
+ display: inline;
+}
+
+.react-tags__selected-tag {
+ display: inline-block;
+ box-sizing: border-box;
+ margin: 0;
+ padding: 6px 8px;
+ border: 0px solid #f5f5f5;
+ border-radius: 2px;
+ background: #f1f1f1;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search {
+ display: inline-block;
+
+ /* new tag border layout */
+ border: 1px dashed #d9d9d9;
+
+ /* match tag layout */
+ line-height: 20px;
+ margin-bottom: 0;
+ padding: 0 7px;
+
+ /* prevent autoresize overflowing the container */
+ max-width: 100%;
+}
+
+.react-tags__search:focus-within {
+ border: 1px solid #000000;
Review Comment:
```suggestion
border: 1px solid ${({ theme }) => theme.colors.grayscale.dark2};
```
Color values should stay within the Superset theme
##########
superset-frontend/src/components/ObjectTags/ObjectTags.css:
##########
@@ -0,0 +1,149 @@
+/**
+ * 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.
+ */
+.ant-tag {
+ color: black;
+}
+
+.react-tags {
+ position: relative;
+ display: inline-block;
+ padding: 1px 0 0 1px;
+ margin: 0 10px;
+ border: 0px solid #f5f5f5;
+ border-radius: 1px;
+
+ /* shared font styles */
+ font-size: 12px;
+ line-height: 1.2;
+
+ /* clicking anywhere will focus the input */
+ cursor: text;
+}
+
+.react-tags__selected {
+ display: inline;
+}
+
+.react-tags__selected-tag {
+ display: inline-block;
+ box-sizing: border-box;
+ margin: 0;
+ padding: 6px 8px;
+ border: 0px solid #f5f5f5;
+ border-radius: 2px;
+ background: #f1f1f1;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search {
+ display: inline-block;
+
+ /* new tag border layout */
+ border: 1px dashed #d9d9d9;
+
+ /* match tag layout */
+ line-height: 20px;
+ margin-bottom: 0;
+ padding: 0 7px;
+
+ /* prevent autoresize overflowing the container */
+ max-width: 100%;
+}
+
+.react-tags__search:focus-within {
+ border: 1px solid #000000;
+}
+
+@media screen and (min-width: 30em) {
+ .react-tags__search {
+ /* this will become the offsetParent for suggestions */
+ position: relative;
+ }
+}
+
+.react-tags__search input {
+ max-width: 150%;
+
+ /* remove styles and layout from this element */
+ margin: 0;
+ margin-left: 0;
+ padding: 0;
+ border: 0;
+ outline: none;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search input::-ms-clear {
+ display: none;
+}
+
+.react-tags__suggestions {
+ position: absolute;
+ top: 100%;
+ left: 0;
+ width: 100%;
+ z-index: 9999;
+}
+
+@media screen and (min-width: 30em) {
+ .react-tags__suggestions {
+ width: 240px;
Review Comment:
```suggestion
width: ${({ theme }) => theme.gridUnit * 60}px;
```
Unit values should stay within the Superset theme
##########
superset-frontend/src/components/Tags/Tag.tsx:
##########
@@ -0,0 +1,84 @@
+/**
+ * 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 { styled, SupersetTheme } from '@superset-ui/core';
+import TagType from 'src/types/TagType';
+import AntdTag from 'antd/lib/tag';
+import React, { useMemo } from 'react';
+import { Tooltip } from 'src/components/Tooltip';
+
+const customTagStyler = (theme: SupersetTheme) => `
+ margin-top: ${theme.gridUnit * 1}px;
+ margin-bottom: ${theme.gridUnit * 1}px;
+ font-size: ${theme.typography.sizes.s}px;
+`;
+
+const StyledTag = styled(AntdTag)`
+ ${({ theme }) => customTagStyler(theme)}}
+`;
Review Comment:
```suggestion
const StyledTag = styled(AntdTag)`
margin-top: ${({ theme }) => theme.gridUnit}px;
margin-bottom: ${({ theme }) => theme.gridUnit}px;
font-size: ${({ theme }) => theme.typography.sizes.s}px;
`;
```
Nit: Combine these `const`s and remove ` * 1`
##########
superset-frontend/src/components/ObjectTags/ObjectTags.css:
##########
@@ -0,0 +1,149 @@
+/**
+ * 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.
+ */
+.ant-tag {
+ color: black;
+}
+
+.react-tags {
+ position: relative;
+ display: inline-block;
+ padding: 1px 0 0 1px;
+ margin: 0 10px;
+ border: 0px solid #f5f5f5;
+ border-radius: 1px;
+
+ /* shared font styles */
+ font-size: 12px;
+ line-height: 1.2;
+
+ /* clicking anywhere will focus the input */
+ cursor: text;
+}
+
+.react-tags__selected {
+ display: inline;
+}
+
+.react-tags__selected-tag {
+ display: inline-block;
+ box-sizing: border-box;
+ margin: 0;
+ padding: 6px 8px;
+ border: 0px solid #f5f5f5;
+ border-radius: 2px;
+ background: #f1f1f1;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search {
+ display: inline-block;
+
+ /* new tag border layout */
+ border: 1px dashed #d9d9d9;
+
+ /* match tag layout */
+ line-height: 20px;
+ margin-bottom: 0;
+ padding: 0 7px;
+
+ /* prevent autoresize overflowing the container */
+ max-width: 100%;
+}
+
+.react-tags__search:focus-within {
+ border: 1px solid #000000;
+}
+
+@media screen and (min-width: 30em) {
Review Comment:
```suggestion
@media screen and (min-width: ${({ theme }) => theme.gridUnit * 7.5}em) {
```
Unit values should stay within the Superset theme
##########
superset-frontend/src/components/ObjectTags/ObjectTags.css:
##########
@@ -0,0 +1,149 @@
+/**
+ * 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.
+ */
+.ant-tag {
+ color: black;
+}
+
+.react-tags {
+ position: relative;
+ display: inline-block;
+ padding: 1px 0 0 1px;
+ margin: 0 10px;
+ border: 0px solid #f5f5f5;
+ border-radius: 1px;
+
+ /* shared font styles */
+ font-size: 12px;
+ line-height: 1.2;
+
+ /* clicking anywhere will focus the input */
+ cursor: text;
+}
+
+.react-tags__selected {
+ display: inline;
+}
+
+.react-tags__selected-tag {
+ display: inline-block;
+ box-sizing: border-box;
+ margin: 0;
+ padding: 6px 8px;
+ border: 0px solid #f5f5f5;
+ border-radius: 2px;
+ background: #f1f1f1;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search {
+ display: inline-block;
+
+ /* new tag border layout */
+ border: 1px dashed #d9d9d9;
+
+ /* match tag layout */
+ line-height: 20px;
+ margin-bottom: 0;
+ padding: 0 7px;
+
+ /* prevent autoresize overflowing the container */
+ max-width: 100%;
+}
+
+.react-tags__search:focus-within {
+ border: 1px solid #000000;
+}
+
+@media screen and (min-width: 30em) {
+ .react-tags__search {
+ /* this will become the offsetParent for suggestions */
+ position: relative;
+ }
+}
+
+.react-tags__search input {
+ max-width: 150%;
+
+ /* remove styles and layout from this element */
+ margin: 0;
+ margin-left: 0;
+ padding: 0;
+ border: 0;
+ outline: none;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search input::-ms-clear {
+ display: none;
+}
+
+.react-tags__suggestions {
+ position: absolute;
+ top: 100%;
+ left: 0;
+ width: 100%;
+ z-index: 9999;
+}
+
+@media screen and (min-width: 30em) {
Review Comment:
```suggestion
@media screen and (min-width: ${({ theme }) => theme.gridUnit * 7.5}em) {
```
Unit values should stay within the Superset theme
##########
superset-frontend/src/components/ObjectTags/ObjectTags.css:
##########
@@ -0,0 +1,149 @@
+/**
+ * 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.
+ */
+.ant-tag {
+ color: black;
+}
+
+.react-tags {
+ position: relative;
+ display: inline-block;
+ padding: 1px 0 0 1px;
+ margin: 0 10px;
+ border: 0px solid #f5f5f5;
+ border-radius: 1px;
+
+ /* shared font styles */
+ font-size: 12px;
+ line-height: 1.2;
+
+ /* clicking anywhere will focus the input */
+ cursor: text;
+}
+
+.react-tags__selected {
+ display: inline;
+}
+
+.react-tags__selected-tag {
+ display: inline-block;
+ box-sizing: border-box;
+ margin: 0;
+ padding: 6px 8px;
+ border: 0px solid #f5f5f5;
+ border-radius: 2px;
+ background: #f1f1f1;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search {
+ display: inline-block;
+
+ /* new tag border layout */
+ border: 1px dashed #d9d9d9;
+
+ /* match tag layout */
+ line-height: 20px;
+ margin-bottom: 0;
+ padding: 0 7px;
+
+ /* prevent autoresize overflowing the container */
+ max-width: 100%;
+}
+
+.react-tags__search:focus-within {
+ border: 1px solid #000000;
+}
+
+@media screen and (min-width: 30em) {
+ .react-tags__search {
+ /* this will become the offsetParent for suggestions */
+ position: relative;
+ }
+}
+
+.react-tags__search input {
+ max-width: 150%;
+
+ /* remove styles and layout from this element */
+ margin: 0;
+ margin-left: 0;
+ padding: 0;
+ border: 0;
+ outline: none;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search input::-ms-clear {
+ display: none;
+}
+
+.react-tags__suggestions {
+ position: absolute;
+ top: 100%;
+ left: 0;
+ width: 100%;
+ z-index: 9999;
+}
+
+@media screen and (min-width: 30em) {
+ .react-tags__suggestions {
+ width: 240px;
+ }
+}
+
+.react-tags__suggestions ul {
+ margin: 4px -1px;
+ padding: 0;
+ list-style: none;
+ background: white;
+ border: 1px solid #d1d1d1;
+ border-radius: 2px;
+ box-shadow: 0 2px 6px rgba(0, 0, 0, 0.2);
+}
+
+.react-tags__suggestions li {
+ border-bottom: 1px solid #ddd;
+ padding: 6px 8px;
+}
+
+.react-tags__suggestions li mark {
+ text-decoration: underline;
+ background: none;
+ font-weight: 600;
Review Comment:
```suggestion
font-weight: ${({ theme }) => theme.typography.weights.bold};
```
Font-weight values should stay within the Superset theme
##########
superset-frontend/src/components/ObjectTags/ObjectTags.css:
##########
@@ -0,0 +1,149 @@
+/**
+ * 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.
+ */
+.ant-tag {
+ color: black;
+}
+
+.react-tags {
+ position: relative;
+ display: inline-block;
+ padding: 1px 0 0 1px;
+ margin: 0 10px;
+ border: 0px solid #f5f5f5;
+ border-radius: 1px;
+
+ /* shared font styles */
+ font-size: 12px;
+ line-height: 1.2;
+
+ /* clicking anywhere will focus the input */
+ cursor: text;
+}
+
+.react-tags__selected {
+ display: inline;
+}
+
+.react-tags__selected-tag {
+ display: inline-block;
+ box-sizing: border-box;
+ margin: 0;
+ padding: 6px 8px;
+ border: 0px solid #f5f5f5;
+ border-radius: 2px;
+ background: #f1f1f1;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search {
+ display: inline-block;
+
+ /* new tag border layout */
+ border: 1px dashed #d9d9d9;
+
+ /* match tag layout */
+ line-height: 20px;
+ margin-bottom: 0;
+ padding: 0 7px;
+
+ /* prevent autoresize overflowing the container */
+ max-width: 100%;
+}
+
+.react-tags__search:focus-within {
+ border: 1px solid #000000;
+}
+
+@media screen and (min-width: 30em) {
+ .react-tags__search {
+ /* this will become the offsetParent for suggestions */
+ position: relative;
+ }
+}
+
+.react-tags__search input {
+ max-width: 150%;
+
+ /* remove styles and layout from this element */
+ margin: 0;
+ margin-left: 0;
+ padding: 0;
+ border: 0;
+ outline: none;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search input::-ms-clear {
+ display: none;
+}
+
+.react-tags__suggestions {
+ position: absolute;
+ top: 100%;
+ left: 0;
+ width: 100%;
+ z-index: 9999;
+}
+
+@media screen and (min-width: 30em) {
+ .react-tags__suggestions {
+ width: 240px;
+ }
+}
+
+.react-tags__suggestions ul {
+ margin: 4px -1px;
+ padding: 0;
+ list-style: none;
+ background: white;
+ border: 1px solid #d1d1d1;
+ border-radius: 2px;
+ box-shadow: 0 2px 6px rgba(0, 0, 0, 0.2);
+}
+
+.react-tags__suggestions li {
+ border-bottom: 1px solid #ddd;
+ padding: 6px 8px;
+}
+
+.react-tags__suggestions li mark {
+ text-decoration: underline;
+ background: none;
+ font-weight: 600;
+}
+
+.react-tags__suggestions li:hover {
+ cursor: pointer;
+ background: #eee;
+}
+
+.react-tags__suggestions li.is-active {
+ background: #b7cfe0;
+}
+
+.react-tags__suggestions li.is-disabled {
+ opacity: 0.5;
Review Comment:
```suggestion
opacity: calc(${({ theme }) => theme.opacity.mediumHeavy});
```
Opacity values should stay within the Superset theme. The available values can be found [here](https://github.com/apache/superset/blob/master/superset-frontend/packages/superset-ui-core/src/style/index.tsx#L128-L133) - I believe the closest value for this would be `mediumHeavy` at 60%.
##########
superset-frontend/src/components/ObjectTags/index.tsx:
##########
@@ -0,0 +1,166 @@
+/**
+ * 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, { useEffect, useState } from 'react';
+import { styled, SupersetClient, t } from '@superset-ui/core';
+import Tag from 'src/types/TagType';
+
+import './ObjectTags.css';
+import { TagsList } from 'src/components/Tags';
+import rison from 'rison';
+import { cacheWrapper } from 'src/utils/cacheWrapper';
+import {
+ ClientErrorObject,
+ getClientErrorObject,
+} from 'src/utils/getClientErrorObject';
+import { deleteTag, fetchTags } from 'src/tags';
+
+export interface ObjectTagsProps {
+ objectType: string;
+ objectId: number;
+ includeTypes: boolean;
+ editMode: boolean;
+ maxTags: number | undefined;
+ onChange?: (tags: Tag[]) => void;
+}
+
+const StyledTagsDiv = styled.div`
+ margin-left: ${({ theme }) => theme.gridUnit * 2}px;
+ max-width: 100%;
+ display: -webkit-flex;
+ display: flex;
+ -webkit-flex-direction: row;
+ -webkit-flex-wrap: wrap;
+`;
+
+const localCache = new Map<string, any>();
+
+const cachedSupersetGet = cacheWrapper(
+ SupersetClient.get,
+ localCache,
+ ({ endpoint }) => endpoint || '',
+);
+
+type SelectTagsValue = {
+ value: string | number | undefined;
+ label: string;
+};
+
+export const tagToSelectOption = (
+ item: Tag & { table_name: string },
+): SelectTagsValue => ({
+ value: item.id,
+ label: item.name,
+});
+
+export const loadTags = async (
+ search: string,
+ page: number,
+ pageSize: number,
+) => {
+ const searchColumn = 'name';
+ const query = rison.encode({
+ filters: [{ col: searchColumn, opr: 'ct', value: search }],
+ page,
+ page_size: pageSize,
+ order_column: searchColumn,
+ order_direction: 'asc',
+ });
+
+ const getErrorMessage = ({ error, message }: ClientErrorObject) => {
+ let errorText = message || error || t('An error has occurred');
+ if (message === 'Forbidden') {
+ errorText = t('You do not have permission to edit this dashboard');
+ }
+ return errorText;
+ };
+
+ return cachedSupersetGet({
+ endpoint: `/api/v1/tag/?q=${query}`,
+ // endpoint: `/api/v1/tags/?q=${query}`,
+ })
+ .then(response => {
+ const data: {
+ label: string;
+ value: string | number;
+ }[] = response.json.result
+ .filter((item: Tag & { table_name: string }) => item.type === 1)
+ .map(tagToSelectOption);
+ return {
+ data,
+ totalCount: response.json.count,
+ };
+ })
+ .catch(async error => {
+ const errorMessage = getErrorMessage(await getClientErrorObject(error));
+ throw new Error(errorMessage);
+ });
+};
+
+export const ObjectTags = ({
+ objectType,
+ objectId,
+ includeTypes,
+ editMode = false,
+ maxTags = undefined,
+ onChange,
+}: ObjectTagsProps) => {
+ const [tags, setTags] = useState<Tag[]>([]);
+
+ useEffect(() => {
+ try {
+ fetchTags(
+ { objectType, objectId, includeTypes },
+ (tags: Tag[]) => setTags(tags),
+ () => {
+ /* TODO: handle error */
+ },
+ );
+ } catch (error: any) {
Review Comment:
Nit: Define `any`
##########
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:
##########
@@ -340,6 +356,25 @@ const PropertiesModal = ({
updateMetadata: false,
});
+ if (isFeatureEnabled(FeatureFlag.TAGGING_SYSTEM)) {
+ // update tags
+ try {
+ fetchTags(
+ {
+ objectType: OBJECT_TYPES.DASHBOARD,
+ objectId: dashboardId,
+ includeTypes: false,
+ },
+ (currentTags: TagType[]) => updateTags(currentTags, tags),
+ () => {
+ /* TODO: handle error */
+ },
+ );
+ } catch (error: any) {
Review Comment:
Nit: Define `any`
##########
superset-frontend/src/components/ObjectTags/index.tsx:
##########
@@ -0,0 +1,166 @@
+/**
+ * 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, { useEffect, useState } from 'react';
+import { styled, SupersetClient, t } from '@superset-ui/core';
+import Tag from 'src/types/TagType';
+
+import './ObjectTags.css';
+import { TagsList } from 'src/components/Tags';
+import rison from 'rison';
+import { cacheWrapper } from 'src/utils/cacheWrapper';
+import {
+ ClientErrorObject,
+ getClientErrorObject,
+} from 'src/utils/getClientErrorObject';
+import { deleteTag, fetchTags } from 'src/tags';
+
+export interface ObjectTagsProps {
+ objectType: string;
+ objectId: number;
+ includeTypes: boolean;
+ editMode: boolean;
+ maxTags: number | undefined;
+ onChange?: (tags: Tag[]) => void;
+}
+
+const StyledTagsDiv = styled.div`
+ margin-left: ${({ theme }) => theme.gridUnit * 2}px;
+ max-width: 100%;
+ display: -webkit-flex;
+ display: flex;
+ -webkit-flex-direction: row;
+ -webkit-flex-wrap: wrap;
+`;
+
+const localCache = new Map<string, any>();
+
+const cachedSupersetGet = cacheWrapper(
+ SupersetClient.get,
+ localCache,
+ ({ endpoint }) => endpoint || '',
+);
+
+type SelectTagsValue = {
+ value: string | number | undefined;
+ label: string;
+};
+
+export const tagToSelectOption = (
+ item: Tag & { table_name: string },
+): SelectTagsValue => ({
+ value: item.id,
+ label: item.name,
+});
+
+export const loadTags = async (
+ search: string,
+ page: number,
+ pageSize: number,
+) => {
+ const searchColumn = 'name';
+ const query = rison.encode({
+ filters: [{ col: searchColumn, opr: 'ct', value: search }],
+ page,
+ page_size: pageSize,
+ order_column: searchColumn,
+ order_direction: 'asc',
+ });
+
+ const getErrorMessage = ({ error, message }: ClientErrorObject) => {
+ let errorText = message || error || t('An error has occurred');
+ if (message === 'Forbidden') {
+ errorText = t('You do not have permission to edit this dashboard');
+ }
+ return errorText;
+ };
+
+ return cachedSupersetGet({
+ endpoint: `/api/v1/tag/?q=${query}`,
+ // endpoint: `/api/v1/tags/?q=${query}`,
+ })
+ .then(response => {
+ const data: {
+ label: string;
+ value: string | number;
+ }[] = response.json.result
+ .filter((item: Tag & { table_name: string }) => item.type === 1)
+ .map(tagToSelectOption);
+ return {
+ data,
+ totalCount: response.json.count,
+ };
+ })
+ .catch(async error => {
+ const errorMessage = getErrorMessage(await getClientErrorObject(error));
+ throw new Error(errorMessage);
+ });
+};
+
+export const ObjectTags = ({
+ objectType,
+ objectId,
+ includeTypes,
+ editMode = false,
+ maxTags = undefined,
+ onChange,
+}: ObjectTagsProps) => {
+ const [tags, setTags] = useState<Tag[]>([]);
+
+ useEffect(() => {
+ try {
+ fetchTags(
+ { objectType, objectId, includeTypes },
+ (tags: Tag[]) => setTags(tags),
+ () => {
+ /* TODO: handle error */
+ },
Review Comment:
Will this be implemented in this PR?
##########
superset-frontend/src/components/ObjectTags/index.tsx:
##########
@@ -0,0 +1,166 @@
+/**
+ * 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, { useEffect, useState } from 'react';
+import { styled, SupersetClient, t } from '@superset-ui/core';
+import Tag from 'src/types/TagType';
+
+import './ObjectTags.css';
+import { TagsList } from 'src/components/Tags';
+import rison from 'rison';
+import { cacheWrapper } from 'src/utils/cacheWrapper';
+import {
+ ClientErrorObject,
+ getClientErrorObject,
+} from 'src/utils/getClientErrorObject';
+import { deleteTag, fetchTags } from 'src/tags';
+
+export interface ObjectTagsProps {
+ objectType: string;
+ objectId: number;
+ includeTypes: boolean;
+ editMode: boolean;
+ maxTags: number | undefined;
+ onChange?: (tags: Tag[]) => void;
+}
+
+const StyledTagsDiv = styled.div`
+ margin-left: ${({ theme }) => theme.gridUnit * 2}px;
+ max-width: 100%;
+ display: -webkit-flex;
+ display: flex;
+ -webkit-flex-direction: row;
+ -webkit-flex-wrap: wrap;
+`;
+
+const localCache = new Map<string, any>();
+
+const cachedSupersetGet = cacheWrapper(
+ SupersetClient.get,
+ localCache,
+ ({ endpoint }) => endpoint || '',
+);
+
+type SelectTagsValue = {
+ value: string | number | undefined;
+ label: string;
+};
+
+export const tagToSelectOption = (
+ item: Tag & { table_name: string },
+): SelectTagsValue => ({
+ value: item.id,
+ label: item.name,
+});
+
+export const loadTags = async (
+ search: string,
+ page: number,
+ pageSize: number,
+) => {
+ const searchColumn = 'name';
+ const query = rison.encode({
+ filters: [{ col: searchColumn, opr: 'ct', value: search }],
+ page,
+ page_size: pageSize,
+ order_column: searchColumn,
+ order_direction: 'asc',
+ });
+
+ const getErrorMessage = ({ error, message }: ClientErrorObject) => {
+ let errorText = message || error || t('An error has occurred');
+ if (message === 'Forbidden') {
+ errorText = t('You do not have permission to edit this dashboard');
+ }
+ return errorText;
+ };
+
+ return cachedSupersetGet({
+ endpoint: `/api/v1/tag/?q=${query}`,
+ // endpoint: `/api/v1/tags/?q=${query}`,
+ })
+ .then(response => {
+ const data: {
+ label: string;
+ value: string | number;
+ }[] = response.json.result
+ .filter((item: Tag & { table_name: string }) => item.type === 1)
+ .map(tagToSelectOption);
+ return {
+ data,
+ totalCount: response.json.count,
+ };
+ })
+ .catch(async error => {
+ const errorMessage = getErrorMessage(await getClientErrorObject(error));
+ throw new Error(errorMessage);
+ });
+};
+
+export const ObjectTags = ({
+ objectType,
+ objectId,
+ includeTypes,
+ editMode = false,
+ maxTags = undefined,
+ onChange,
+}: ObjectTagsProps) => {
+ const [tags, setTags] = useState<Tag[]>([]);
+
+ useEffect(() => {
+ try {
+ fetchTags(
+ { objectType, objectId, includeTypes },
+ (tags: Tag[]) => setTags(tags),
+ () => {
+ /* TODO: handle error */
+ },
+ );
+ } catch (error: any) {
+ console.log(error);
+ }
+ }, [objectType, objectId, includeTypes]);
+
+ const onDelete = (tagIndex: number) => {
+ deleteTag(
+ { objectType, objectId },
+ tags[tagIndex],
+ () => setTags(tags.filter((_, i) => i !== tagIndex)),
+ () => {
+ /* TODO: handle error */
+ },
Review Comment:
Will this be implemented in this PR?
##########
superset-frontend/src/views/CRUD/tags/TagsTable.tsx:
##########
@@ -0,0 +1,124 @@
+/**
+ * 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, { useState, useEffect } from 'react';
+import moment from 'moment';
+import { t, styled } from '@superset-ui/core';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { fetchObjects } from '../../../tags';
+import Loading from '../../../components/Loading';
+
+const TagsTableContainer = styled.div`
+ text-align: left;
+ border-radius: ${({ theme }) => theme.gridUnit * 1}px 0;
Review Comment:
```suggestion
border-radius: ${({ theme }) => theme.borderRadius}px 0;
```
Border-radius values should stay within the Superset theme, the current value is 4 as seen [here](https://github.com/apache/superset/blob/master/superset-frontend/packages/superset-ui-core/src/style/index.tsx#L55).
##########
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:
##########
@@ -521,6 +557,67 @@ const PropertiesModal = ({
}
}, [dashboardInfo, dashboardTitle, form]);
+ useEffect(() => {
+ if (!isFeatureEnabled(FeatureFlag.TAGGING_SYSTEM)) return;
+ try {
+ fetchTags(
+ {
+ objectType: OBJECT_TYPES.DASHBOARD,
+ objectId: dashboardId,
+ includeTypes: false,
+ },
+ (tags: TagType[]) => setTags(tags),
+ () => {
+ /* TODO: handle error */
+ },
+ );
+ } catch (error: any) {
Review Comment:
Nit: Define `any`
##########
superset-frontend/src/components/Tags/TagsList.test.tsx:
##########
@@ -0,0 +1,76 @@
+/**
+ * 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 { render, screen } from 'spec/helpers/testing-library';
+import TagsList, { TagsListProps } from './TagsList';
+
+const testTags = [
+ {
+ name: 'example-tag1',
+ id: 1,
+ },
+ {
+ name: 'example-tag2',
+ id: 2,
+ },
+ {
+ name: 'example-tag3',
+ id: 3,
+ },
+ {
+ name: 'example-tag4',
+ id: 4,
+ },
+ {
+ name: 'example-tag5',
+ id: 5,
+ },
+];
+
+const mockedProps: TagsListProps = {
+ tags: testTags,
+ onDelete: undefined,
+ maxTags: 5,
+};
+
+const findAllTags = () => screen.getAllByRole('link')! as HTMLElement[];
+
+test('should render', () => {
+ const { container } = render(<TagsList {...mockedProps} />);
+ expect(container).toBeInTheDocument();
+ // console.log(screen.getAllByRole("tag"));
Review Comment:
Nit: remove comment
##########
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:
##########
@@ -618,6 +715,31 @@ const PropertiesModal = ({
</p>
</Col>
</Row>
+ <Row gutter={16}>
+ <Col xs={24} md={12}>
+ <h3 style={{ marginTop: '1em' }}>{t('Tags')}</h3>
Review Comment:
```suggestion
<h3 css={{ marginTop: '1em' }}>{t('Tags')}</h3>
```
Use `css` for any inline styles
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -182,6 +224,71 @@ function PropertiesModal({
setName(slice.slice_name || '');
}, [slice.slice_name]);
+ useEffect(() => {
+ if (!isFeatureEnabled(FeatureFlag.TAGGING_SYSTEM)) return;
+ try {
+ fetchTags(
+ {
+ objectType: OBJECT_TYPES.CHART,
+ objectId: slice.slice_id,
+ includeTypes: false,
+ },
+ (tags: TagType[]) => setTags(tags),
+ () => {
+ /* TODO: handle error */
+ },
+ );
+ } catch (error: any) {
Review Comment:
Nit: Describe `any`
##########
superset-frontend/src/explore/components/ExploreChartHeader/index.jsx:
##########
@@ -60,6 +61,11 @@ const saveButtonStyles = theme => css`
}
`;
+// const StyledButtons = styled.span`
+// display: flex;
+// align-items: center;
+// `;
+
Review Comment:
Nit: Remove commented code
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -314,6 +421,26 @@ function PropertiesModal({
)}
</StyledHelpBlock>
</FormItem>
+ {isFeatureEnabled(FeatureFlag.TAGGING_SYSTEM) && (
+ <h3 style={{ marginTop: '1em' }}>{t('Tags')}</h3>
Review Comment:
```suggestion
<h3 css={{ marginTop: '1em' }}>{t('Tags')}</h3>
```
Use `css` for any inline styles.
##########
superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx:
##########
@@ -339,6 +347,25 @@ function DashboardList(props: DashboardListProps) {
disableSortBy: true,
size: 'xl',
},
+ {
+ Cell: ({
+ row: {
+ original: { tags = [] },
+ },
+ }: any) => (
Review Comment:
Nit: Describe `any`
##########
superset-frontend/src/views/CRUD/tags/Tags.tsx:
##########
@@ -0,0 +1,95 @@
+/**
+ * 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, { useEffect, useState } from 'react';
+import { styled } from '@superset-ui/core';
+import Tag from 'src/types/TagType';
+import { StringParam, useQueryParam } from 'use-query-params';
+import withToasts from 'src/components/MessageToasts/withToasts';
+import SelectControl from 'src/explore/components/controls/SelectControl';
+import { fetchSuggestions } from 'src/tags';
+import TagsTable from './TagsTable';
+
+const TagsContainer = styled.div`
+ background-color: ${({ theme }) => theme.colors.grayscale.light4};
+ .select-control {
+ margin-left: ${({ theme }) => theme.gridUnit * 4}px;
+ margin-right: ${({ theme }) => theme.gridUnit * 4}px;
+ margin-bottom: ${({ theme }) => theme.gridUnit * 2}px;
+ }
+ .select-control-label {
+ text-transform: uppercase;
+ font-size: ${({ theme }) => theme.gridUnit * 3}px;
+ color: ${({ theme }) => theme.colors.grayscale.base};
+ margin-bottom: ${({ theme }) => theme.gridUnit * 1}px;
+ }
+`;
+
+const TagsNav = styled.div`
+ height: 50px;
Review Comment:
```suggestion
height: ${({ theme }) => theme.gridUnit * 12.5}px;
```
Unit values should stay within the Superset theme
##########
superset-frontend/src/components/ObjectTags/ObjectTags.css:
##########
@@ -0,0 +1,149 @@
+/**
+ * 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.
+ */
+.ant-tag {
+ color: black;
+}
+
+.react-tags {
+ position: relative;
+ display: inline-block;
+ padding: 1px 0 0 1px;
+ margin: 0 10px;
+ border: 0px solid #f5f5f5;
+ border-radius: 1px;
+
+ /* shared font styles */
+ font-size: 12px;
+ line-height: 1.2;
+
+ /* clicking anywhere will focus the input */
+ cursor: text;
+}
+
+.react-tags__selected {
+ display: inline;
+}
+
+.react-tags__selected-tag {
+ display: inline-block;
+ box-sizing: border-box;
+ margin: 0;
+ padding: 6px 8px;
+ border: 0px solid #f5f5f5;
+ border-radius: 2px;
+ background: #f1f1f1;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search {
+ display: inline-block;
+
+ /* new tag border layout */
+ border: 1px dashed #d9d9d9;
+
+ /* match tag layout */
+ line-height: 20px;
+ margin-bottom: 0;
+ padding: 0 7px;
+
+ /* prevent autoresize overflowing the container */
+ max-width: 100%;
+}
+
+.react-tags__search:focus-within {
+ border: 1px solid #000000;
+}
+
+@media screen and (min-width: 30em) {
+ .react-tags__search {
+ /* this will become the offsetParent for suggestions */
+ position: relative;
+ }
+}
+
+.react-tags__search input {
+ max-width: 150%;
+
+ /* remove styles and layout from this element */
+ margin: 0;
+ margin-left: 0;
+ padding: 0;
+ border: 0;
+ outline: none;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search input::-ms-clear {
+ display: none;
+}
+
+.react-tags__suggestions {
+ position: absolute;
+ top: 100%;
+ left: 0;
+ width: 100%;
+ z-index: 9999;
+}
+
+@media screen and (min-width: 30em) {
+ .react-tags__suggestions {
+ width: 240px;
+ }
+}
+
+.react-tags__suggestions ul {
+ margin: 4px -1px;
+ padding: 0;
+ list-style: none;
+ background: white;
+ border: 1px solid #d1d1d1;
+ border-radius: 2px;
+ box-shadow: 0 2px 6px rgba(0, 0, 0, 0.2);
Review Comment:
```suggestion
box-shadow: 0 ${({ theme }) => theme.gridUnit * 0.5}px
${({ theme }) => theme.gridUnit * 1.5}px
rgba(0, 0, 0, ${({ theme }) => theme.opacity.light});
```
Opacity and unit values should stay within the Superset theme. The available opacity values can be found [here](https://github.com/apache/superset/blob/master/superset-frontend/packages/superset-ui-core/src/style/index.tsx#L128-L133) - I believe the closest value would be either `light` at 10% or `mediumLight` at 35%.
##########
superset-frontend/src/views/CRUD/chart/ChartList.tsx:
##########
@@ -366,6 +370,27 @@ function ChartList(props: ChartListProps) {
disableSortBy: true,
size: 'xl',
},
+ {
+ Cell: ({
+ row: {
+ original: { tags = [] },
+ },
+ }: any) => (
Review Comment:
Nit: Describe `any`
##########
superset-frontend/src/views/CRUD/tags/TagsTable.tsx:
##########
@@ -0,0 +1,124 @@
+/**
+ * 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, { useState, useEffect } from 'react';
+import moment from 'moment';
+import { t, styled } from '@superset-ui/core';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { fetchObjects } from '../../../tags';
+import Loading from '../../../components/Loading';
+
+const TagsTableContainer = styled.div`
+ text-align: left;
+ border-radius: ${({ theme }) => theme.gridUnit * 1}px 0;
+ margin: 0 ${({ theme }) => theme.gridUnit * 4}px;
+ .table {
+ table-layout: fixed;
+ }
+ .td {
+ width: 33%;
+ }
+`;
+
+interface TaggedObject {
+ id: number;
+ type: string;
+ name: string;
+ url: string;
+ changed_on: moment.MomentInput;
+ created_by: number | undefined;
+ creator: string;
+}
+
+interface TaggedObjects {
+ dashboard: TaggedObject[];
+ chart: TaggedObject[];
+ query: TaggedObject[];
+}
+
+interface TagsTableProps {
+ search?: string;
+}
+
+export default function TagsTable({ search = '' }: TagsTableProps) {
+ const [objects, setObjects] = useState<TaggedObjects>({
+ dashboard: [],
+ chart: [],
+ query: [],
+ });
+
+ useEffect(() => {
+ const fetchResults = (search: string) => {
+ fetchObjects(
+ { tags: search, types: null },
+ (data: TaggedObject[]) => {
+ const objects = { dashboard: [], chart: [], query: [] };
+ data.forEach(object => {
+ objects[object.type].push(object);
+ });
+ setObjects(objects);
+ },
+ (error: Response) => {
+ console.log(error.json());
+ },
+ );
+ };
+ fetchResults(search);
+ }, [search]);
+
+ const renderTable = (type: any) => {
+ const data = objects[type].map((o: TaggedObject) => ({
+ [type]: <a href={o.url}>{o.name}</a>,
+ // eslint-disable-next-line react/no-danger
+ creator: <div dangerouslySetInnerHTML={{ __html: o.creator }} />,
Review Comment:
```suggestion
creator: <div>o.creator</div>,
```
I feel like it would be safer to set this in a div normally, is there a reason for using `dangerouslySetInnerHTML` instead?
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -148,6 +170,25 @@ function PropertiesModal({
}[]
).map(o => o.value);
}
+ if (isFeatureEnabled(FeatureFlag.TAGGING_SYSTEM)) {
+ // update tags
+ try {
+ fetchTags(
+ {
+ objectType: OBJECT_TYPES.CHART,
+ objectId: slice.slice_id,
+ includeTypes: false,
+ },
+ (currentTags: TagType[]) => updateTags(currentTags, tags),
+ () => {
+ /* TODO: handle error */
+ },
+ );
+ } catch (error: any) {
Review Comment:
Nit: Describe `any`
##########
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:
##########
@@ -521,6 +557,67 @@ const PropertiesModal = ({
}
}, [dashboardInfo, dashboardTitle, form]);
+ useEffect(() => {
+ if (!isFeatureEnabled(FeatureFlag.TAGGING_SYSTEM)) return;
+ try {
+ fetchTags(
+ {
+ objectType: OBJECT_TYPES.DASHBOARD,
+ objectId: dashboardId,
+ includeTypes: false,
+ },
+ (tags: TagType[]) => setTags(tags),
+ () => {
+ /* TODO: handle error */
+ },
Review Comment:
Will this be implemented in this PR?
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -182,6 +224,71 @@ function PropertiesModal({
setName(slice.slice_name || '');
}, [slice.slice_name]);
+ useEffect(() => {
+ if (!isFeatureEnabled(FeatureFlag.TAGGING_SYSTEM)) return;
+ try {
+ fetchTags(
+ {
+ objectType: OBJECT_TYPES.CHART,
+ objectId: slice.slice_id,
+ includeTypes: false,
+ },
+ (tags: TagType[]) => setTags(tags),
+ () => {
+ /* TODO: handle error */
+ },
Review Comment:
Will this be implemented in this PR?
##########
superset-frontend/src/components/Tags/Tag.tsx:
##########
@@ -0,0 +1,84 @@
+/**
+ * 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 { styled, SupersetTheme } from '@superset-ui/core';
+import TagType from 'src/types/TagType';
+import AntdTag from 'antd/lib/tag';
+import React, { useMemo } from 'react';
+import { Tooltip } from 'src/components/Tooltip';
+
+const customTagStyler = (theme: SupersetTheme) => `
+ margin-top: ${theme.gridUnit * 1}px;
+ margin-bottom: ${theme.gridUnit * 1}px;
+ font-size: ${theme.typography.sizes.s}px;
+`;
+
+const StyledTag = styled(AntdTag)`
+ ${({ theme }) => customTagStyler(theme)}}
+`;
+
+const Tag = ({
+ name,
+ id,
+ index = undefined,
+ onDelete = null,
+ editable = false,
+ onClick = null,
+}: TagType) => {
+ const isLongTag = useMemo(() => name.length > 20, [name]);
+
+ const handleClose = () => (index ? onDelete(index) : null);
+
+ const tagElem = (
+ <>
+ {editable ? (
+ <StyledTag
+ key={id}
+ closable={editable}
+ onClose={handleClose}
+ color="blue"
+ >
+ {isLongTag ? `${name.slice(0, 20)}...` : name}
+ </StyledTag>
+ ) : (
+ <StyledTag role="link" key={id} onClick={onClick}>
+ {id ? (
+ <a href={`/superset/tags/?tags=${name}`}>
Review Comment:
Could you set this to open in a new tab instead of the same page?
##########
superset-frontend/src/components/ObjectTags/ObjectTags.css:
##########
@@ -0,0 +1,149 @@
+/**
+ * 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.
+ */
+.ant-tag {
+ color: black;
+}
+
+.react-tags {
+ position: relative;
+ display: inline-block;
+ padding: 1px 0 0 1px;
+ margin: 0 10px;
+ border: 0px solid #f5f5f5;
Review Comment:
I see that this border is being set to `0px` but also setting a color. Is the intent to hide the border or set it as a different color?
##########
superset-frontend/src/components/ObjectTags/ObjectTags.css:
##########
@@ -0,0 +1,149 @@
+/**
+ * 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.
+ */
+.ant-tag {
+ color: black;
+}
+
+.react-tags {
+ position: relative;
+ display: inline-block;
+ padding: 1px 0 0 1px;
+ margin: 0 10px;
+ border: 0px solid #f5f5f5;
+ border-radius: 1px;
+
+ /* shared font styles */
+ font-size: 12px;
+ line-height: 1.2;
+
+ /* clicking anywhere will focus the input */
+ cursor: text;
+}
+
+.react-tags__selected {
+ display: inline;
+}
+
+.react-tags__selected-tag {
+ display: inline-block;
+ box-sizing: border-box;
+ margin: 0;
+ padding: 6px 8px;
+ border: 0px solid #f5f5f5;
+ border-radius: 2px;
+ background: #f1f1f1;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search {
+ display: inline-block;
+
+ /* new tag border layout */
+ border: 1px dashed #d9d9d9;
+
+ /* match tag layout */
+ line-height: 20px;
+ margin-bottom: 0;
+ padding: 0 7px;
+
+ /* prevent autoresize overflowing the container */
+ max-width: 100%;
+}
+
+.react-tags__search:focus-within {
+ border: 1px solid #000000;
+}
+
+@media screen and (min-width: 30em) {
+ .react-tags__search {
+ /* this will become the offsetParent for suggestions */
+ position: relative;
+ }
+}
+
+.react-tags__search input {
+ max-width: 150%;
+
+ /* remove styles and layout from this element */
+ margin: 0;
+ margin-left: 0;
+ padding: 0;
+ border: 0;
+ outline: none;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search input::-ms-clear {
+ display: none;
+}
+
+.react-tags__suggestions {
+ position: absolute;
+ top: 100%;
+ left: 0;
+ width: 100%;
+ z-index: 9999;
+}
+
+@media screen and (min-width: 30em) {
+ .react-tags__suggestions {
+ width: 240px;
+ }
+}
+
+.react-tags__suggestions ul {
+ margin: 4px -1px;
+ padding: 0;
+ list-style: none;
+ background: white;
Review Comment:
```suggestion
background: ${({ theme }) => theme.colors.grayscale.light5};
```
Color values should stay within the Superset theme
##########
superset-frontend/src/components/ObjectTags/ObjectTags.css:
##########
@@ -0,0 +1,149 @@
+/**
+ * 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.
+ */
+.ant-tag {
+ color: black;
+}
+
+.react-tags {
+ position: relative;
+ display: inline-block;
+ padding: 1px 0 0 1px;
+ margin: 0 10px;
+ border: 0px solid #f5f5f5;
+ border-radius: 1px;
+
+ /* shared font styles */
+ font-size: 12px;
+ line-height: 1.2;
+
+ /* clicking anywhere will focus the input */
+ cursor: text;
+}
+
+.react-tags__selected {
+ display: inline;
+}
+
+.react-tags__selected-tag {
+ display: inline-block;
+ box-sizing: border-box;
+ margin: 0;
+ padding: 6px 8px;
+ border: 0px solid #f5f5f5;
+ border-radius: 2px;
+ background: #f1f1f1;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search {
+ display: inline-block;
+
+ /* new tag border layout */
+ border: 1px dashed #d9d9d9;
+
+ /* match tag layout */
+ line-height: 20px;
+ margin-bottom: 0;
+ padding: 0 7px;
+
+ /* prevent autoresize overflowing the container */
+ max-width: 100%;
+}
+
+.react-tags__search:focus-within {
+ border: 1px solid #000000;
+}
+
+@media screen and (min-width: 30em) {
+ .react-tags__search {
+ /* this will become the offsetParent for suggestions */
+ position: relative;
+ }
+}
+
+.react-tags__search input {
+ max-width: 150%;
+
+ /* remove styles and layout from this element */
+ margin: 0;
+ margin-left: 0;
+ padding: 0;
+ border: 0;
+ outline: none;
+
+ /* match the font styles */
+ font-size: inherit;
+ line-height: inherit;
+}
+
+.react-tags__search input::-ms-clear {
+ display: none;
+}
+
+.react-tags__suggestions {
+ position: absolute;
+ top: 100%;
+ left: 0;
+ width: 100%;
+ z-index: 9999;
+}
+
+@media screen and (min-width: 30em) {
+ .react-tags__suggestions {
+ width: 240px;
+ }
+}
+
+.react-tags__suggestions ul {
+ margin: 4px -1px;
+ padding: 0;
+ list-style: none;
+ background: white;
+ border: 1px solid #d1d1d1;
+ border-radius: 2px;
+ box-shadow: 0 2px 6px rgba(0, 0, 0, 0.2);
+}
+
+.react-tags__suggestions li {
+ border-bottom: 1px solid #ddd;
+ padding: 6px 8px;
+}
+
+.react-tags__suggestions li mark {
+ text-decoration: underline;
+ background: none;
+ font-weight: 600;
+}
+
+.react-tags__suggestions li:hover {
+ cursor: pointer;
+ background: #eee;
Review Comment:
Color values should stay within the Superset theme. The available color values can be found [here](https://github.com/apache/superset/blob/master/superset-frontend/packages/superset-ui-core/src/style/index.tsx#L56-L127).
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -148,6 +170,25 @@ function PropertiesModal({
}[]
).map(o => o.value);
}
+ if (isFeatureEnabled(FeatureFlag.TAGGING_SYSTEM)) {
+ // update tags
+ try {
+ fetchTags(
+ {
+ objectType: OBJECT_TYPES.CHART,
+ objectId: slice.slice_id,
+ includeTypes: false,
+ },
+ (currentTags: TagType[]) => updateTags(currentTags, tags),
+ () => {
+ /* TODO: handle error */
+ },
Review Comment:
Will this be implemented in this PR?
##########
superset-frontend/src/components/ObjectTags/index.tsx:
##########
@@ -0,0 +1,166 @@
+/**
+ * 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, { useEffect, useState } from 'react';
+import { styled, SupersetClient, t } from '@superset-ui/core';
+import Tag from 'src/types/TagType';
+
+import './ObjectTags.css';
+import { TagsList } from 'src/components/Tags';
+import rison from 'rison';
+import { cacheWrapper } from 'src/utils/cacheWrapper';
+import {
+ ClientErrorObject,
+ getClientErrorObject,
+} from 'src/utils/getClientErrorObject';
+import { deleteTag, fetchTags } from 'src/tags';
+
+export interface ObjectTagsProps {
+ objectType: string;
+ objectId: number;
+ includeTypes: boolean;
+ editMode: boolean;
+ maxTags: number | undefined;
+ onChange?: (tags: Tag[]) => void;
+}
+
+const StyledTagsDiv = styled.div`
+ margin-left: ${({ theme }) => theme.gridUnit * 2}px;
+ max-width: 100%;
+ display: -webkit-flex;
+ display: flex;
+ -webkit-flex-direction: row;
+ -webkit-flex-wrap: wrap;
Review Comment:
```suggestion
display: flex;
flex-direction: row;
flex-wrap: wrap;
```
Emotion uses the [Stylis CSS Preprocessor](https://github.com/thysultan/stylis), which handles these prefixes on runtime. See [this thread](https://github.com/emotion-js/emotion/issues/1523#issuecomment-535865527) for more info
##########
superset-frontend/src/components/ObjectTags/ObjectTags.css:
##########
@@ -0,0 +1,149 @@
+/**
+ * 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.
+ */
+.ant-tag {
+ color: black;
+}
+
+.react-tags {
+ position: relative;
+ display: inline-block;
+ padding: 1px 0 0 1px;
+ margin: 0 10px;
+ border: 0px solid #f5f5f5;
+ border-radius: 1px;
Review Comment:
```suggestion
border-radius: ${({ theme }) => theme.borderRadius}px;
```
Border-radius values should stay within the Superset theme, the current value is 4 as seen [here](https://github.com/apache/superset/blob/master/superset-frontend/packages/superset-ui-core/src/style/index.tsx#L55).
##########
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:
##########
@@ -340,6 +356,25 @@ const PropertiesModal = ({
updateMetadata: false,
});
+ if (isFeatureEnabled(FeatureFlag.TAGGING_SYSTEM)) {
+ // update tags
+ try {
+ fetchTags(
+ {
+ objectType: OBJECT_TYPES.DASHBOARD,
+ objectId: dashboardId,
+ includeTypes: false,
+ },
+ (currentTags: TagType[]) => updateTags(currentTags, tags),
+ () => {
+ /* TODO: handle error */
+ },
Review Comment:
Will this be implemented in this 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] cccs-RyanK commented on pull request #20876: feat: Frontend tagging
Posted by "cccs-RyanK (via GitHub)" <gi...@apache.org>.
cccs-RyanK commented on PR #20876:
URL: https://github.com/apache/superset/pull/20876#issuecomment-1437453575
> Yey, congrats @cccs-RyanK, this is a really great feature. Are we clear to merge this on Tuesday?
All good from my end!
--
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] lyndsiWilliams commented on pull request #20876: feat: Frontend tagging
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on PR #20876:
URL: https://github.com/apache/superset/pull/20876#issuecomment-1347344748
/testenv up FEATURE_TAGGING_SYSTEM=true
--
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] cccs-RyanK commented on a diff in pull request #20876: feat: Frontend tagging
Posted by GitBox <gi...@apache.org>.
cccs-RyanK commented on code in PR #20876:
URL: https://github.com/apache/superset/pull/20876#discussion_r1047390140
##########
superset-frontend/src/views/CRUD/tags/TagList.tsx:
##########
@@ -0,0 +1,332 @@
+/**
+ * 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 { t } from '@superset-ui/core';
+import React, { useMemo, useCallback } from 'react';
+import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags';
+import {
+ createFetchRelated,
+ createErrorHandler,
+ Actions,
+} from 'src/views/CRUD/utils';
+import { useListViewResource } from 'src/views/CRUD/hooks';
+import ConfirmStatusChange from 'src/components/ConfirmStatusChange';
+import SubMenu, { SubMenuProps } from 'src/views/components/SubMenu';
+import ListView, {
+ ListViewProps,
+ Filters,
+ FilterOperator,
+} from 'src/components/ListView';
+import { dangerouslyGetItemDoNotUse } from 'src/utils/localStorageHelpers';
+import withToasts from 'src/components/MessageToasts/withToasts';
+import Icons from 'src/components/Icons';
+import { Tooltip } from 'src/components/Tooltip';
+import FacePile from 'src/components/FacePile';
+import { Link } from 'react-router-dom';
+import { deleteTags } from 'src/tags';
+import { Tag as AntdTag } from 'antd';
+import { Tag } from '../types';
+import TagCard from './TagCard';
+
+const PAGE_SIZE = 25;
+
+interface TagListProps {
+ addDangerToast: (msg: string) => void;
+ addSuccessToast: (msg: string) => void;
+ user: {
+ userId: string | number;
+ firstName: string;
+ lastName: string;
+ };
+}
+
+function TagList(props: TagListProps) {
+ const {
+ addDangerToast,
+ addSuccessToast,
+ user: { userId },
+ } = props;
+
+ const {
+ state: {
+ loading,
+ resourceCount: tagCount,
+ resourceCollection: tags,
+ bulkSelectEnabled,
+ },
+ hasPerm,
+ fetchData,
+ toggleBulkSelect,
+ refreshData,
+ } = useListViewResource<Tag>('tag', t('tag'), addDangerToast);
+
+ // TODO: Fix usage of localStorage keying on the user id
+ const userKey = dangerouslyGetItemDoNotUse(userId?.toString(), null);
Review Comment:
This is used in the other CRUD views (ex: ChartList.tsx) for the thumbnails and looking through that area of the code it looks like the proper way to do it isn't set up for this quite yet. I copied the TODO comment along with it to keep track of it along with the other views. I can remove it from this list, but I had it in there to keep consistent with the other CRUD views.
--
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] cccs-RyanK commented on a diff in pull request #20876: feat: Frontend tagging
Posted by GitBox <gi...@apache.org>.
cccs-RyanK commented on code in PR #20876:
URL: https://github.com/apache/superset/pull/20876#discussion_r1047399076
##########
superset-frontend/src/views/CRUD/tags/TagList.tsx:
##########
@@ -0,0 +1,332 @@
+/**
+ * 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 { t } from '@superset-ui/core';
+import React, { useMemo, useCallback } from 'react';
+import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags';
+import {
+ createFetchRelated,
+ createErrorHandler,
+ Actions,
+} from 'src/views/CRUD/utils';
+import { useListViewResource } from 'src/views/CRUD/hooks';
+import ConfirmStatusChange from 'src/components/ConfirmStatusChange';
+import SubMenu, { SubMenuProps } from 'src/views/components/SubMenu';
+import ListView, {
+ ListViewProps,
+ Filters,
+ FilterOperator,
+} from 'src/components/ListView';
+import { dangerouslyGetItemDoNotUse } from 'src/utils/localStorageHelpers';
+import withToasts from 'src/components/MessageToasts/withToasts';
+import Icons from 'src/components/Icons';
+import { Tooltip } from 'src/components/Tooltip';
+import FacePile from 'src/components/FacePile';
+import { Link } from 'react-router-dom';
+import { deleteTags } from 'src/tags';
+import { Tag as AntdTag } from 'antd';
+import { Tag } from '../types';
+import TagCard from './TagCard';
+
+const PAGE_SIZE = 25;
+
+interface TagListProps {
+ addDangerToast: (msg: string) => void;
+ addSuccessToast: (msg: string) => void;
+ user: {
+ userId: string | number;
+ firstName: string;
+ lastName: string;
+ };
+}
+
+function TagList(props: TagListProps) {
+ const {
+ addDangerToast,
+ addSuccessToast,
+ user: { userId },
+ } = props;
+
+ const {
+ state: {
+ loading,
+ resourceCount: tagCount,
+ resourceCollection: tags,
+ bulkSelectEnabled,
+ },
+ hasPerm,
+ fetchData,
+ toggleBulkSelect,
+ refreshData,
+ } = useListViewResource<Tag>('tag', t('tag'), addDangerToast);
+
+ // TODO: Fix usage of localStorage keying on the user id
+ const userKey = dangerouslyGetItemDoNotUse(userId?.toString(), null);
+
+ const canDelete = hasPerm('can_write');
+
+ const initialSort = [{ id: 'changed_on_delta_humanized', desc: true }];
+
+ function handleTagsDelete(
+ tags: Tag[],
+ callback: (text: string) => void,
+ error: (text: string) => void,
+ ) {
+ // TODO what permissions need to be checked here?
+ deleteTags(tags, callback, error);
+ refreshData();
+ }
+
+ const columns = useMemo(
+ () => [
+ {
+ Cell: ({
+ row: {
+ original: { name: tagName },
+ },
+ }: any) => (
Review Comment:
The `any` typing on the Cells in the tables are consistent with the other CRUD views (ex: ChartList, DashboardList). Along with the typing on ListViewProps. I can update the typing to be more specific if you think that is best
--
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] rusackas commented on pull request #20876: feat: Frontend tagging
Posted by GitBox <gi...@apache.org>.
rusackas commented on PR #20876:
URL: https://github.com/apache/superset/pull/20876#issuecomment-1397454952
Thanks for the force-restart. Keeping an eye on CI now. 🤞
--
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] hughhhh commented on a diff in pull request #20876: feat: Frontend tagging
Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh commented on code in PR #20876:
URL: https://github.com/apache/superset/pull/20876#discussion_r1088026004
##########
superset/dashboards/api.py:
##########
@@ -212,20 +215,37 @@ def ensure_thumbnails_enabled(self) -> Optional[Response]:
edit_columns = add_columns
search_columns = (
- "created_by",
- "changed_by",
- "dashboard_title",
- "id",
- "owners",
- "published",
- "roles",
- "slug",
+ (
+ "created_by",
+ "changed_by",
+ "dashboard_title",
+ "id",
+ "owners",
+ "published",
+ "roles",
+ "slug",
+ )
+ if not is_feature_enabled("TAGGING_SYSTEM")
Review Comment:
just append `tags` `if is_feature_enabled("TAGGING_SYSTEM")` lets use positive logic 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] github-actions[bot] commented on pull request #20876: feat: Frontend tagging
Posted by github-actions.
github-actions[bot] commented on PR #20876:
URL: https://github.com/apache/superset/pull/20876#issuecomment-1410860770
@yousoph Ephemeral environment spinning up at http://35.91.84.185:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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