Skip to content

feat: adding new filter icon #1185

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

Merged
merged 2 commits into from
Oct 13, 2021
Merged

feat: adding new filter icon #1185

merged 2 commits into from
Oct 13, 2021

Conversation

palbizu
Copy link
Contributor

@palbizu palbizu commented Oct 11, 2021

Description

Adding new filter icon

Testing

Visual testing

image

Checklist:

  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules

Documentation

Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.

@palbizu palbizu requested a review from a team as a code owner October 11, 2021 23:14
@palbizu palbizu requested a review from jake-bassett October 11, 2021 23:14
@@ -51,6 +51,7 @@ const iconsRootPath = 'assets/icons';
{ key: IconType.Eye, url: `${iconsRootPath}/eye.svg` },
{ key: IconType.FileCode, url: `${iconsRootPath}/file-code.svg` },
{ key: IconType.Filter, url: `${iconsRootPath}/filter.svg` },
{ key: IconType.FilterSpecial, url: `${iconsRootPath}/filter-special.svg` },
Copy link
Contributor

Choose a reason for hiding this comment

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

When would we use this filter icon instead of the other one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed the mocks, should I keep the old filter icon here or replace it with the new-one?

Copy link
Contributor

Choose a reason for hiding this comment

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

Where are we using the existing filter icon? Can you share what it looks like?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are not using a button. Just in the tables but here a similar example in an input
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. I could see us needing both as we have different types of filtering, but was curious about the distinction so we could find a better name.

Let's go ahead and merge this with the current name. Once we get a feel for the UX usage, we should rename.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could rename the existing one to 'drilldown-filter'. That is the purpose of that icon in the UI now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure @arjunlalb. Icon renamed.

@codecov
Copy link

codecov bot commented Oct 11, 2021

Codecov Report

Merging #1185 (1d5b52a) into main (8c4e99c) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1185   +/-   ##
=======================================
  Coverage   85.02%   85.02%           
=======================================
  Files         832      832           
  Lines       17224    17224           
  Branches     2239     2239           
=======================================
  Hits        14645    14645           
  Misses       2547     2547           
  Partials       32       32           
Impacted Files Coverage Δ
...ts/assets-library/src/icons/icon-library.module.ts 100.00% <ø> (ø)

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 8c4e99c...1d5b52a. Read the comment docs.

@github-actions

This comment has been minimized.

jake-bassett
jake-bassett previously approved these changes Oct 12, 2021
@github-actions

This comment has been minimized.

@jake-bassett jake-bassett merged commit 4d802d3 into main Oct 13, 2021
@jake-bassett jake-bassett deleted the NewFilterIcon branch October 13, 2021 16:52
@github-actions
Copy link

Unit Test Results

    4 files  271 suites   18m 32s ⏱️
970 tests 970 ✔️ 0 💤 0 ❌
977 runs  977 ✔️ 0 💤 0 ❌

Results for commit 4d802d3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants