-
-
Notifications
You must be signed in to change notification settings - Fork 33
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
Implement filtering on Name, Address, Alt# and Created By column headings #1828
Conversation
…into 1647-implement-filters-on-text-columns
@@ -4,8 +4,11 @@ import Button from "../../Button/Button"; | |||
import RadioButton from "../../UI/RadioButton"; | |||
import "react-datepicker/dist/react-datepicker.css"; | |||
import { MdClose } from "react-icons/md"; | |||
import ReactSelect from "react-select"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Long story: The styling of drop-downs was inconsistent across browsers and operating systems (Issue #1785). @bilalbg was not able to fix it with just CSS and resorted to introducing the react-select library to replace the standard react component and creating a custom react component UniversalSelect which wrapped react-select and applied styles to get the appearance the designers wanted. Unfortunately, we dropped the ball on testing his PR, and though the list populated correctly, the selected value would not display the selected value, and this broke every page in the app with drop-downs. I temporarily switched everything back to to get through Tuesday's stakeholder meeting, but then looked into why the UniversalSelect wasn't working and created Issue #1817 and PR #1818 to fix it, and switched the existing usages back to . I revised the value and onChange props in UniversalSelect to work like they do in the component. This may or may not be the best option, but I was just trying to get it working for the usages that were in the app. At this time, we want all the drop-downs to use the UniversalSelect component, so we get the consistent styling. We can discuss if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @entrotech for fixing UniversalSelect. The value and onChange props work fine, but since UniversalSelect does not accept a placeholder
prop, I cannot place the search Icon next to the placeholder text. Please advise.
|
||
/> */} | ||
<ReactSelect | ||
options={selectOptions.map(text => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the list would be more user-friendly if it was sorted alphabetically. You can do it here or in the ProjectPage component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is completed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I made a few comments in the PR that I think should be addressed before merging.
Other thoughts about the design that I don't think should hold up merging your changes, but probably should be discussed with the designers:
- The list often extends beyond the boundary of the drop-down window. I had this problem with the date filtering popup as well. Not sure whether we should do anything about that.
- The prototype had checkboxes next to each item in the list - I guess they were thinking you might want to select multiple choices from the list, but I'm not sure that's really needed.
- It's also not clear to me whether you should be forced to pick one match from the list. It might be useful to be able to just type a string, NOT select anything from the list, and apply the filter to get any projects where the column matches the substring.
…into 1647-implement-filters-on-text-columns Pull in changes from #1818
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After trying this out, I don't like how it works with a drop-down list box.
- You are forced to pick an item from the drop-down list, when I would prefer that you could just type a search string, and it would include any values that include the search string.
- Once you select an item, and apply the filter, it still uses the the string selected as a substring in doing the matching, so does kind of match by a substring, but only lets you pick substrings that happen to be the complete string in one of the projects.
- If you select an item to filter by, and then re-open the pop-up, you can't really unselect the pre-selected item in an intuitive way. You have to press Reset, then Apply to unlock the drop-down to show you other choices.
IMO, it was probably a mistake to use a listbox in the first place, and a better choice would be an auto-complete combo box, so the control allows any text, and the combo drop-down is just a convenience, in case they happen to be looking for an exact match.
We can do any or all of the following:
Merge this as-is and then we can demonstrate the problems to the PMs and design team.
We could go off-script and come up with something we like and propose that to the team as an alternative.
We could just kick this back to the design team with comments about what we don't like and see if they can come up with something better.
What do you think?
@entrotech Regarding 1 and 2, do you think we could use async to enable the user to type a search string and then have matching options load, then the user would have the option of selecting from the list? I agree that clearing the filter is a bit awkward and it might be worthwhile to consider a design change. If you wish to merge the PR so that it is more readily available for demonstration, that is fine, but I'm also happy to continue working on it until it meets the customer's needs. |
Fixes #1647
What changes did you make?
ProjectTableColumnHeader
andTextPopup
components are now called with a new propuniqueValues
, an array that includes sorted unique non-null values from all the currently displayed projects (all pages) for the specific project property.Why did you make the changes (we will use this info to test)?
TextPopup
component didn't have access to project properties which are required for generating match options.Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)
Visuals before changes are applied
Visuals after changes are applied