Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: List available badges on Home & Browse Pages #1535

Merged
merged 1 commit into from
Oct 15, 2021

Conversation

mgorsk1
Copy link
Contributor

@mgorsk1 mgorsk1 commented Oct 15, 2021

Summary of Changes

Introduces Available badges on landing page and Browse badges in Browse page.

Available badges component on frontend is rendered only if /badges returns non-empty list of badges
So far all badges are shown under Available badges header - we could go with grouping badges into badge categories (header would contain name of category and list of badges would show badges under this category)

This is follow-up to RFC: amundsen-io/rfcs#44


This PR introduces following changes to UI:

  • Home Page:
    image

  • Browse Page:
    image

Tests

  • done

Documentation

  • n/a

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"
  • PR includes a summary of 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 functions and the classes in the PR contain docstrings that explain what it does

Signed-off-by: Mariusz Górski <gorskimariusz13@gmail.com>
@mgorsk1 mgorsk1 force-pushed the feat/all_badges_list branch from d5d8acf to db4460a Compare October 15, 2021 13:58
@mgorsk1 mgorsk1 merged commit 5685791 into main Oct 15, 2021
@youngyjd
Copy link
Member

youngyjd commented Nov 3, 2021

It looks like both column badges and table badges are showing in the homepage. I think it maybe better to just display table badges on homepage? clicking on a column badge which takes me to a search result page literally does not show any table

@mgorsk1
Copy link
Contributor Author

mgorsk1 commented Nov 3, 2021

that's a good point @youngyjd, however it is consistent with what happens when you click on column badge (in table view).

If we were to change this to table badges only I think the easiest way would be to do it in the metadata proxy - show only badges connected to tables (modify cypher query). unfortunately badge is generic type and there is no other distinction between table/column badges.

<h1 className="header-title">{BROWSE_BADGES_TITLE}</h1>
<hr className="header-hr" />
<label className="section-label">
<span className="section-title title-2">{AVAILABLE_BADGES_TITLE}</span>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'title-X' class is deprecated, please add the stiles to 'section-title' with an extend or use classes like text-body-w2

@import 'typography';

.badges-browse-section {
margin: 32px 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try to make this values a multiple of the grid or use the spacer variables


.badges-browse-section-short {
.available-badges-header-title {
@extend %text-title-w1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the styling we want

@import 'variables';

.flag {
border-radius: 5px;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a variable for this

[class^='badge-overlay-'] {
font-size: $font-size-base;
text-align: center;
padding: 0.2em 0.6em 0.3em;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, we are not using ems in Amundsen for the moment

}
}
}
border-radius: 10px;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

variable too

amommendes pushed a commit to amommendes/amundsen that referenced this pull request Jan 21, 2022
Signed-off-by: Mariusz Górski <gorskimariusz13@gmail.com>
Signed-off-by: Amom Mendes <amommendes@hotmail.com>
ozandogrultan pushed a commit to deliveryhero/amundsen that referenced this pull request Apr 28, 2022
Signed-off-by: Mariusz Górski <gorskimariusz13@gmail.com>
zacr pushed a commit to SaltIO/amundsen that referenced this pull request May 13, 2022
Signed-off-by: Mariusz Górski <gorskimariusz13@gmail.com>
hansadriaans pushed a commit to DataChefHQ/amundsen that referenced this pull request Jun 30, 2022
Signed-off-by: Mariusz Górski <gorskimariusz13@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:frontend From the Frontend folder
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants