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

Update global filter with new UX requests #1119

Merged
merged 3 commits into from
Jan 14, 2021

Conversation

karelhala
Copy link
Contributor

New updates to global filter

UX requested new things added to the global filter

  • Remove "All Workloads" option
  • Change SAP from radio button to checkbox
  • Change textbox words from "Search Tags" to "Filter Results"

@karelhala karelhala requested a review from a team January 12, 2021 14:47
@codecov-io
Copy link

codecov-io commented Jan 13, 2021

Codecov Report

Merging #1119 (e9f7a5f) into master (27ef898) will increase coverage by 0.08%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1119      +/-   ##
==========================================
+ Coverage   52.92%   53.01%   +0.08%     
==========================================
  Files          65       65              
  Lines        1315     1311       -4     
  Branches      259      258       -1     
==========================================
- Hits          696      695       -1     
+ Misses        493      491       -2     
+ Partials      126      125       -1     
Impacted Files Coverage Δ
src/js/App/GlobalFilter/GlobalFilter.js 1.72% <0.00%> (+0.08%) ⬆️
src/js/App/GlobalFilter/constants.js 71.15% <ø> (-0.55%) ⬇️

@@ -9,7 +9,7 @@ import { Chip, ChipGroup } from '@patternfly/react-core/dist/js/components/ChipG
import { Button } from '@patternfly/react-core/dist/js/components/Button';
import { Tooltip } from '@patternfly/react-core/dist/js/components/Tooltip';
import TagsModal from './TagsModal';
import { workloads, updateSelected, storeFilter, generateFilter, selectWorkloads } from './constants';
import { workloads, updateSelected, storeFilter, generateFilter } from './constants';
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the selectWorkloads to be used anywhere else. Can we delete the function declaration as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}
}, [selectedTags, isAllowed()]);

useEffect(() => {
const sapTag = workloads?.[0]?.tags?.[1];
const sapTag = workloads?.[0]?.tags?.[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

I am just curious about this one. The SAP tag is always the first tag? And if so can this be refactored so it's a bit nicer on the eye?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the initial mocks, there was SAP and some other workload, hence the array. It's a static array passed from constants. I'm thinking about exporting just the SAP tag itself and bypass the workloads.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it would definitely make more sense. We can do it in a different PR though.

@Hyperkid123 Hyperkid123 merged commit 99564a1 into RedHatInsights:master Jan 14, 2021
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.

3 participants