Add basic account filter/search #120
Conversation
This adds a very rudimentary search function for recent logins. It's a simple case insenstive search. It doesn't re-order results or anything fancy like that. This should make it easier to quickly find accounts if you have given them helpful aliases. Currently it searches both the name and url. Searching the URL may not make sense, but it didn't hurt in my tests. GH-118
There was a problem hiding this comment.
I'd like @tquetano-r7 to take a look at this if he has a chance too.
| const mapDispatchToProps = () => ({ | ||
| }); | ||
|
|
||
| export const RecentLogins = connect(mapStateToProps, mapDispatchToProps)(RecentLoginsComponent); |
There was a problem hiding this comment.
I don't think you need to connect the Redux state here.
| const metadataUrls = this.props.metadataUrls; | ||
| const filterText = this.state.filterText; | ||
|
|
||
| if (filterText === undefined || filterText === '') { |
There was a problem hiding this comment.
This can be simplified as if (!filterText) {.
|
Thanks @dgreene-r7 - I disconnected redux state and simplified the empty check. |
| }); | ||
| }; | ||
|
|
||
| filterMetadataUrls = () => { |
There was a problem hiding this comment.
theres no reason this needs to be an instance field ... this could easily be a pure standalone function if passed the metadata / filterText as parameters.
| <Login | ||
| key={url} | ||
| pretty={name} | ||
| profileId={i} |
There was a problem hiding this comment.
using the iteration index as an ID of any kind is very dangerous and de-optimizes react rerender cycles, u should consider using a consistent unique value.
There was a problem hiding this comment.
Great call-out. This would have actually broken deleting a profile when URLs were filtered because the delete uses the iteration index.
Unfortunately, this is how the code was when I found it. I've attempted to fix by starting to store a UUID with each profile and then referencing the UUID instead of the iteration index: c5470b1
Thoughts?
| filterText: '', | ||
| }; | ||
|
|
||
| handleFilterInputChange = ({target: {value}}) => { |
There was a problem hiding this comment.
u may consider renaming value to take advantage of shorthand (and define the assignment contract in the initial line):
handleFilterInputChange = ({currentTarget: {value: filterText}}) => this.setState({filterText});also, its considered best practice to use currentTarget instead of target.
This makes deletes a little safer and prevents us from needing to use the iteration index for deletes.
|
I think I've addressed the code review comments. If @dgreene-r7 and @tquetano-r7 could take one more look I'd appreciate it. Especially this commit: c5470b1, which I added as a way to ensure deletes still work even when filtering URLs. |
This adds a very rudimentary search function for recent logins. It's a simple case insenstive search. It doesn't re-order results or anything fancy like that. This should make it easier to quickly find accounts if you have given them helpful aliases.
Currently it searches both the name and url. Searching the URL may not make sense, but it didn't hurt in my tests.
GH-118