feat: Show more tags in browse and home #532

allisonsuarez · 2020-08-03T22:50:41Z

Summary of Changes

Amundsen currently shows a small list of curated tags on both the homepage and browse. This is built using a FE configuration which explicitly lists out a set of approved tags to show on this widget. Since this list is manually curated but not updated, this list isn't necessarily the best set of tags to show. Additionally the "/browse" page shows the same content as the homepage, which isn't very useful. We should remove the manually curated list of tags.

The browse component should have two parts:

"Popular Tags" section that shows the top X tags based on usage.
"All Tags" section that shows all other tags not in the "Top Tags" section.

No curated tags specified: get popular tags sorted by usage (X number)
Curated tags specified: display curated tags under popular tags (not sorted)
Config showAllTags is true: same behavior as before on home page, show curated followed by a line and then all other tags under, this doesn't affect Browse page anymore.

Tests

What tests did you add or modify and why? If no tests were added or modified, explain why. Remove this line

Documentation

What documentation did you add or modify and why? Add any relevant links then remove this line

CheckList

Make sure you have checked all steps below to ensure a timely review.

PR title addresses the issue accurately and concisely. Example: "Updates the version of Flask to v1.0.2"
- In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.
PR includes a summary of changes, including screenshots of any UI changes.
PR adds unit tests, updates existing unit tests, OR documents why no test additions or modifications are needed.
In case of new functionality, my PR adds documentation that describes how to use it.
- All the public python functions and the classes in the PR contain docstrings that explain what it does
PR passes all tests documented in the developer guide

codecov-commenter · 2020-08-03T22:51:53Z

Codecov Report

Merging #532 into master will decrease coverage by 5.44%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #532      +/-   ##
==========================================
- Coverage   81.16%   75.71%   -5.45%     
==========================================
  Files         230       42     -188     
  Lines        5718     1816    -3902     
  Branches      676      174     -502     
==========================================
- Hits         4641     1375    -3266     
+ Misses       1009      391     -618     
+ Partials       68       50      -18

Impacted Files	Coverage Δ
amundsen_application/static/js/utils/logUtils.ts
...tic/js/components/TableDetail/WriterLink/index.tsx
...tion/static/js/components/TableDetail/constants.ts
...application/static/js/ducks/announcements/types.ts
...atic/js/components/common/LoadingSpinner/index.tsx
...js/components/TableDetail/WatermarkLabel/index.tsx
...ts/common/ResourceListItem/TableListItem/index.tsx
...pplication/static/js/ducks/popularTables/api/v0.ts
...s/components/DashboardPage/QueryListItem/index.tsx
...ts/SearchPage/SearchFilter/FilterSection/index.tsx
... and 174 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ffea88d...cf7c7f4. Read the comment docs.

allisonsuarez · 2020-08-06T20:19:50Z

amundsen_application/static/js/components/common/TagsList/index.tsx

  render() {
    const { isLoading, curatedTags, otherTags } = this.props;

    if (isLoading) {
      return <ShimmeringTagListLoader />;
    }

+    const filteredTags = otherTags.filter((tag) => {


Should I filter out tags with usage of 0 in the Curated Tags section as well? Was hesitant because it affects previous behavior, on the upside it might help keeping curated tags less stale.

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

…ction getPopularTags, commented out all tests for TagsList because rewrite will be necessary Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

…onality implemented on TagsList, still need to do some work to make section not show up when no tags exist Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

… page, as well as two section brose page displayz' Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

…ented title switching base don curated vs popular tags Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

…ed tags with 0 Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

Golodhros

Looks good!
Some minor comments

amundsen_application/static/js/components/BrowsePage/index.tsx

amundsen_application/static/js/components/BrowsePage/tests/index.spec.tsx

amundsen_application/static/js/components/HomePage/styles.scss

amundsen_application/static/js/components/Tags/TagsList/index.tsx

amundsen_application/static/js/components/Tags/TagsList/styles.scss

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

amundsen_application/static/js/components/common/Tags/TagsList/index.spec.tsx

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

amundsen_application/static/js/components/common/Tags/TagsList/index.spec.tsx

amundsen_application/static/js/components/BrowsePage/index.tsx

amundsen_application/static/js/components/HomePage/index.spec.tsx

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

Golodhros

LGTM

Golodhros

LGTM

ttannis

Can we remove all TODO comments and instead create issues for them in the public repo, with details and tagged as "help wanted" and good first issue? Thanks!

ttannis · 2020-08-17T17:48:09Z

amundsen_application/static/js/fixtures/metadata/tagsList.ts

@@ -0,0 +1,73 @@
+export const allTestTags = [


nit: This is just tags, but be careful of copy pasting our staging dummy data and committing that code. It's best to commit our own dummy data always.

* commit 'd2f222ea5cb648fb4a9d9bd2e242a3b36281098d': (63 commits) feat: Update 'table view', 'alumni' and SLA status badges (amundsen-io#595) build: Adding betterer to workflow (amundsen-io#598) build(deps): bump flask-restful from 0.3.7 to 0.3.8 (amundsen-io#553) chore: Removes 84 unused variables (amundsen-io#600) chore: remove travis badge and update doc link (amundsen-io#599) Fixes error on TableDetail on Dev (amundsen-io#597) chore: Bump version for pypi release. (amundsen-io#596) feat: Announcements in Homepage (amundsen-io#591) build(deps-dev): bump @babel/plugin-proposal-logical-assignment-operators (amundsen-io#574) build(deps): [security] bump lodash in /amundsen_application/static (amundsen-io#587) build(deps-dev): bump @babel/plugin-proposal-export-namespace-from (amundsen-io#588) build(deps-dev): bump lint-staged in /amundsen_application/static (amundsen-io#590) chore: fix docker push action (amundsen-io#593) feat: add github actions for FE (amundsen-io#592) chore: Re-useable OwnerEditor (amundsen-io#548) feat: Show more tags in browse and home (amundsen-io#532) Organizes, themes and adds typography stories (amundsen-io#578) docs: describe storybook (amundsen-io#577) Fixes TypeScript issues with sagas and saga tests (amundsen-io#573) build(deps-dev): bump @babel/core in /amundsen_application/static (amundsen-io#558) ... # Conflicts: # amundsen_application/api/utils/metadata_utils.py # amundsen_application/oidc_config.py # amundsen_application/static/js/components/TableDetail/ColumnList/index.tsx # amundsen_application/static/js/components/TableDetail/ColumnListItem/index.tsx # amundsen_application/static/js/components/TableDetail/constants.ts # amundsen_application/static/js/components/TableDetail/index.tsx # amundsen_application/static/js/config/index.spec.ts # amundsen_application/static/js/ducks/tableMetadata/reducer.ts # amundsen_application/static/js/fixtures/globalState.ts # amundsen_application/static/js/fixtures/metadata/table.ts # amundsen_application/static/js/interfaces/TableMetadata.ts # docs/application_config.md # requirements.txt # setup.py

allisonsuarez marked this pull request as draft August 3, 2020 22:51

allisonsuarez commented Aug 6, 2020

View reviewed changes

allisonsuarez changed the title ~~[feat] Show more tags in browse and home~~ feat: Show more tags in browse and home Aug 6, 2020

allisonsuarez marked this pull request as ready for review August 6, 2020 20:23

allisonsuarez added 15 commits August 11, 2020 12:31

renamed section header to Popular Tags

6b52bc9

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

trying things out

79bb1b5

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

set up logic for different exec paths, still have to test sorting fun…

03e7586

…ction getPopularTags, commented out all tests for TagsList because rewrite will be necessary Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

functionaltiy for home page with both curated and popular tags functi…

88be098

…onality implemented on TagsList, still need to do some work to make section not show up when no tags exist Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

completed basic implementation of home page tasks with link to browse…

7c948a2

… page, as well as two section brose page displayz' Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

tested and fixed some edge cases

ce60042

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

lint

f773093

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

sorted tags alphabetically within categories

e25c29a

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

some refactoring, adjusted spacing on homepage, fixed styling, implem…

e42f1ed

…ented title switching base don curated vs popular tags Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

lint and test fix

8037aa8

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

fixed issue to filter out 0 value tags from other tags, keeping curat…

1218cbf

…ed tags with 0 Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

refactor, still need to fix some errors

978a661

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

this works

fbda5ef

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

replaced a with Link component

f048afa

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

resolved lint issues

eed74c7

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

allisonsuarez force-pushed the asuarezmiranda-more-tags branch from 3fec7e8 to eed74c7 Compare August 11, 2020 19:31

Golodhros reviewed Aug 11, 2020

View reviewed changes

fixed minor issues from PR feedback

b1c141a

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

allisonsuarez force-pushed the asuarezmiranda-more-tags branch from ddc99c9 to b1c141a Compare August 11, 2020 20:44

made tag_name key instead of index

c8779cd

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

allisonsuarez force-pushed the asuarezmiranda-more-tags branch from e78dbd1 to c8779cd Compare August 11, 2020 20:51

allisonsuarez added 4 commits August 11, 2020 14:49

browse page test issue

4f22c71

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

trying to use mount, not working

ae3fe2f

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

still having issues when using mount on TagsList for test

a09cb69

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

here are some working tests for tagsList

73af0eb

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

Golodhros reviewed Aug 14, 2020

View reviewed changes

amundsen_application/static/js/components/common/Tags/TagsList/index.spec.tsx Outdated Show resolved Hide resolved

used map for allChildren

3d71d9e

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

Golodhros reviewed Aug 14, 2020

View reviewed changes

amundsen_application/static/js/components/common/Tags/TagsList/index.spec.tsx Outdated Show resolved Hide resolved

Golodhros reviewed Aug 14, 2020

View reviewed changes

amundsen_application/static/js/components/BrowsePage/index.tsx Outdated Show resolved Hide resolved

Golodhros reviewed Aug 14, 2020

View reviewed changes

amundsen_application/static/js/components/HomePage/index.spec.tsx Outdated Show resolved Hide resolved

allisonsuarez added 4 commits August 14, 2020 16:12

added fixture fro huge tags list

e03650a

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

fixes/cleanup

161bf4e

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

added test for shimmer loading render

b9ae60e

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

lint

34bd2f2

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

allisonsuarez requested review from ttannis, dikshathakur3119 and Golodhros August 14, 2020 23:24

Golodhros previously approved these changes Aug 14, 2020

View reviewed changes

resolved merge conflict

c7cda63

allisonsuarez dismissed Golodhros’s stale review via c7cda63 August 14, 2020 23:47

allisonsuarez requested a review from Golodhros August 15, 2020 00:01

Golodhros approved these changes Aug 17, 2020

View reviewed changes

allisonsuarez merged commit 4f6164d into master Aug 17, 2020

allisonsuarez deleted the asuarezmiranda-more-tags branch August 17, 2020 17:17

ttannis reviewed Aug 17, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Show more tags in browse and home #532

feat: Show more tags in browse and home #532

allisonsuarez commented Aug 3, 2020 •

edited

Loading

codecov-commenter commented Aug 3, 2020 •

edited

Loading

allisonsuarez Aug 6, 2020

Golodhros left a comment

Golodhros left a comment

Golodhros left a comment

ttannis left a comment

ttannis Aug 17, 2020

feat: Show more tags in browse and home #532

feat: Show more tags in browse and home #532

Conversation

allisonsuarez commented Aug 3, 2020 • edited Loading

Summary of Changes

Tests

Documentation

CheckList

codecov-commenter commented Aug 3, 2020 • edited Loading

Codecov Report

allisonsuarez Aug 6, 2020

Choose a reason for hiding this comment

Golodhros left a comment

Choose a reason for hiding this comment

Golodhros left a comment

Choose a reason for hiding this comment

Golodhros left a comment

Choose a reason for hiding this comment

ttannis left a comment

Choose a reason for hiding this comment

ttannis Aug 17, 2020

Choose a reason for hiding this comment

allisonsuarez commented Aug 3, 2020 •

edited

Loading

codecov-commenter commented Aug 3, 2020 •

edited

Loading