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

fix(filter): Removed placeholder from within dropdown… #449

Merged
merged 1 commit into from Aug 24, 2018

Conversation

dtaylor113
Copy link
Member

@dtaylor113 dtaylor113 commented Aug 22, 2018

… and show current value in dropdown

Before:

  • Placeholder text within dropdown
  • Selected/Applied filter value not reflected in dropdown
    filter_before

After:

  • Placeholder text removed from dropdown
  • Selected/Applied filter value reflected in dropdown
  • Dropdown remembers previously applied filter value
    filter_after

CC @mcarrano, @akaRem

Fixes #418

@patternfly-build
Copy link
Contributor

patternfly-build commented Aug 22, 2018

Deploy preview for patternfly-ng ready!

Built with commit 0bc3664

https://deploy-preview-449--patternfly-ng.netlify.com

@dtaylor113 dtaylor113 force-pushed the filter-updates branch 4 times, most recently from 0ed2bb5 to 690301e Compare August 22, 2018 17:32
@mcarrano
Copy link
Member

@dtaylor113 This looks promising. But I cannot seem to get to the preview link. Can you check it?

@dlabrecq
Copy link
Member

@mcarrano Try the link again. It worked for me, but takes a bit to load

@mcarrano
Copy link
Member

This looks great, @dtaylor113 . My only question is whether it can be made so that the text is only italicized when it is the placeholder. At all other times, the selection should not be italicized. If this is not possible, then I think I would remove the italicized style altogether.

As for the multi-select variant we talked about yesterday, here is a quick mockup to show what this could look like. Let me know what you think.

multi-select filter

@dtaylor113 dtaylor113 force-pushed the filter-updates branch 2 times, most recently from 5d03903 to be458ea Compare August 23, 2018 14:04
@dtaylor113
Copy link
Member Author

I was thinking the same thing @mcarrano :-) Here's the updated screenshots:
image
image

@dlabrecq
Copy link
Member

@mcarrano I may have encountered an issue with the pattern.

  1. Choose a birth month option -- the selection is shown in the menu
  2. Now change the filter field from Birth Month to Name
  3. Change the filter field back to Birth Month

Although the filter has been applied, the filter selection is no longer reflected in the menu. Wondering if the menu selection is expected to reflect the applied filter in this scenario?

new gif

@dtaylor113
Copy link
Member Author

dtaylor113 commented Aug 23, 2018

Although the filter has been applied, the filter selection is no longer reflected in the menu

Hi @dlabrecq, I raised this in the middle of patternfly/patternfly#1116. I'm still not sure how complicated it will be to have the dropdown remember it's state -especially if we eventually support multiple select values. It also depends on whether we also support enforcing single select or allow dropdown filters to be accumulative.

I'll spend a little more time looking into this issue. I think for now we are always enforcing single select/applied filter for dropdowns. Meaning a dropdown always replaces an existing filter tag. In this scenario it makes sense to reset the dropdown to the applied filter value.

@dtaylor113
Copy link
Member Author

@mcarrano, @dlabrecq Dropdowns now remember previously applied filter values. I've updated this PRs Description and screenshots to show this. -thanks

@mcarrano
Copy link
Member

This is looking good @dtaylor113 Do you want to try to incorporate the multi-select here also or would you rather consider that in a separate PR?

@dtaylor113 dtaylor113 changed the title [WIP] fix(filter): Removed placeholder from within dropdown… fix(filter): Removed placeholder from within dropdown… Aug 24, 2018
@dtaylor113
Copy link
Member Author

This is looking good @dtaylor113 Do you want to try to incorporate the multi-select here also or would you rather consider that in a separate PR?

Separate PR please! I need to apply these changes to patternfly-react next, then I'd work on multi-select. -thanks

@mcarrano, @dlabrecq please approve this PR when/if ready -thanks

Copy link
Member

@dlabrecq dlabrecq left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

Looks great @dtaylor113 . I will open a separate issue for the multi-select version of this control.

@dtaylor113
Copy link
Member Author

I will open a separate issue for the multi-select version of this control.

Thanks @mcarrano, please be sure to indicate that multi-select should use 'Grouped Filter Chips'.

@dlabrecq dlabrecq merged commit 0dac7f6 into patternfly:master Aug 24, 2018
@patternfly-build
Copy link
Contributor

🎉 This PR is included in version 4.5.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

None yet

4 participants