LPS-122662 Make vocabularies not searchable #473
Conversation
|
To conserve resources, the PR Tester does not automatically run for every pull. If your code changes were already tested in another pull, reference that pull in this pull so the test results can be analyzed. If your pull was never tested, comment "ci:test" to run the PR Tester for this pull. |
|
ci:test:relevant |
|
ci:test:sf |
We can give it to you if you want it!! 😘 |
|
I think we may be open to exchange, are you interested in iframe or weather portlet? 😏 |
liferay-continuous-integration
added
ci:test:sf - pending
ci:test:sf - success
and removed
ci:test:sf - pending
labels
✔️ ci:test:sf - 1 out of 1 jobs passed in 4 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPS-122662_disabling_nodes_search 1 Successful Jobs:For more details click here. |
| @@ -84,7 +84,12 @@ function filterNodes(nodes, filterQuery) { | |||
| const filteredNodes = []; | |||
|
|
|||
| nodes.forEach((node) => { | |||
| if (node.name.toLowerCase().indexOf(filterQuery) !== -1) { | |||
| const isSearchable = node.searchable ?? true; | |||
There was a problem hiding this comment.
I don't have a better idea right away and probably lack a lot of context but this feels a bit of an ad-hoc API that will fail to deliver soon enough...
Is it guaranteed that you'll always be able to add the searchable attribute to every node in your model? This might be coming from a remote request which we might have no control over.
Not sure if possible, but could we make the filterFn the prop instead? I see you already have something like vocabulary: true, which maybe we could use together with a custom filter function to avoid adding those to the search?
Something like:
<TreeView
filter={(node, query) => !node.vocabulary && node.name.toLowerCase().indexOf(filterQuery) !== -1}
nodes={vocabularies}
/>There was a problem hiding this comment.
I guess in those cases you could decorate the response 🤔 but nonetheless, I think that your suggestions would make it more flexible, so I'll go for it :) Thanks!
😂 We basically took iframe already with Remote Apps, so you might need to take this 😂 |
|
Jenkins Build:test-portal-source-format#3404 |
victorg1991
force-pushed
the
LPS-122662_disabling_nodes_search
branch
from
ec01e9d
to
13d3bc2
Compare
|
Hey @jbalsas I've updated the PR :) |
| filter={(node, filterQuery) => | ||
| !node.vocabulary && | ||
| node.name | ||
| .toLowerCase() | ||
| .indexOf(filterQuery.toLowerCase()) !== | ||
| -1 | ||
| } |
There was a problem hiding this comment.
Just thinking out loud here, but the API has a bit of a "redundant" feel to it, and not just because the word filter appears in two props (which might be confusing for people and definitely requires documentation, unless you think it will be obvious to people what filter and filterQuery are, and how to use them, and when).
The thing is, the callsite here has access to filterQuery already, and it's then passing in a function that will get that same filterQuery passed back into it, when it could have just accessed it via the closure. Among other things this function we pass in is going to call toLowerCase() again and again on the passed in filterQuery in a loop (due to the forEach() calls), always producing exactly the same result.
All of which is to say I wonder if we couldn't do this with a single prop (PropTypes.oneOfType[PropTypes.func. PropTypes.string]): if passed a string, behave exactly as before, if passed a func, use it to filter; eg.
const query = filterQuery.toLowerCase();
<Treeview
filter={(node) =>
!node.vocabulary &&
node.name.toLowerCase().includes(query)
}
/>There was a problem hiding this comment.
I agree this approach is better, the only thing that worries me is that is a breaking change, but I'm not sure if someone outside Liferay is using it 🤔 Should we just go for it removing the old prop? or maybe leave the prop for compatibility?
What do you think?
@jbalsas what do we usually do with breaking changes in javascript API?
There was a problem hiding this comment.
I assumed filterQuery was local to the treeview... based on this I guess the search input is controlled from the outside, which makes sense.
I think we could deprecate the filterQuery prop but keep it in this version. If filter is passed, we ignore filterQuery, otherwise it works as before.
I think it's almost certain no one is using this outside of Liferay, but we could be using it inside in different ways. The cost of keeping this for backwards compatibility seems relatively small, so I guess we can keep it around.
There was a problem hiding this comment.
the only thing that worries me is that is a breaking change
Ah, I didn't mean to propose a breaking change — instead of replacing filterQuery with filter, I meant to augment the behavior of the existing prop in a non-breaking way; the rename is just me being careless and imprecise. (Of course, filter is a better name, but 🤷♂️ ).
I think it's almost certain no one is using this outside of Liferay
If that's true, then maybe we shouldn't worry about this?...
but we could be using it inside in different ways. The cost of keeping this for backwards compatibility seems relatively small, so I guess we can keep it around.
Well, that is the cost I was trying to avoid (of having two confusingly similar props) — the thing is, complexity accrues one prop at a time, and before you know it you've got a ghastly, Frankenstein API that is hard to use and hard to evolve.
There was a problem hiding this comment.
If that's true, then maybe we shouldn't worry about this?...
I wouldn't necessarily worry about this right now... just check for internal usages...
There was a problem hiding this comment.
Is there any protocol for deprecating props like that? If none exist I think it may be better to leave the name of the property unchanged to avoid confusion 🤔 what do you think?
|
Jenkins Build:test-portal-acceptance-pullrequest(master)#6303 |
victorg1991
force-pushed
the
LPS-122662_disabling_nodes_search
branch
2 times, most recently
from
0791d0b
to
8259799
Compare
|
Well, here we go again @jbalsas @wincent I'm not really sure about this commit 7543666 I have to do that because treeview has a "search" mode enabled when searching that flatten the nodes, I have to do that comparison because there isn't other way to know right now if we are in search mode or not. Before this change filteredQuery containing a falsy value was the flag for enabling/disabling this kind of view. Let me know what do you think :) PD: I'm starting to prefer the first version (simply including a new searchable prop to the nodes) 😂 |
|
I'm realizing that I've missed updating the usages, but I think I'll wait until we have the definitive version of this |
There was a problem hiding this comment.
I'm not really sure about this commit 7543666
I have to do that because treeview has a "search" mode enabled when searching that flatten the nodes, I have to do that comparison because there isn't other way to know right now if we are in search mode or not. Before this change filteredQuery containing a falsy value was the flag for enabling/disabling this kind of view.
Couldn't you just not pass a filter function if the query is empty? Then you wouldn't need to make such invasive changes inside the Treeview; eg.
- Pass a string: behavior is as before.
- Pass a function: let caller take care of filtering.
- Pass nothing: don't filter.
| @@ -39,21 +39,23 @@ function SelectCategory({ | |||
| nodes, | |||
| }) { | |||
| const flattenedNodes = useMemo(() => { | |||
| if (nodes.length === 1 && nodes[0].vocabulary) { | |||
| if (nodes.length === 1 && nodes[0].vocabulary && nodes[0].id !== '0') { | |||
There was a problem hiding this comment.
I don't really get this change, what's the reason for it?
There was a problem hiding this comment.
The view look like this:
I've changed the model to add the property vocabulary: true to the root node, but doing so this code is removing that root, so I've just had to add this check here to preserve the root after setting the property
Another option would be to check for id !== '0' in the filter function, but I think it isn't much of an improvement
Also I am not sure where this code is used, I've seen that this code can be improved a lot, but I've wanted to make minimal changes here in order to fix the bug, and revisit this later, (when removing aui stuff for example)
I did something of the sorts in my first approach but it didn't look right, I think it is not that intuitive to rely on passing null to the query in case you want to filter, isn't it? |
As I understand it, the old system did this:
With the proposed change, instead of a query we can now pass a function or a string, and every single call site in liferay-portal except one will behave exactly as it used to, because they will all still be passing a string. Seeing as strings can be "empty" or "non-empty", and that signals whether a search is happening, and functions cannot (ie. they're always functions), that suggests that the only way for our special function-based case to signal "I'm not really searching" is to not pass the function (ie. what I suggested in my last comment), or to do this with two props instead of one. I have a mild preference for the first option (because reasoning about combinations of props is hard, and if you're not careful you can create situations where multiple props are in mutually incompatible/conflicting states, compared to a single prop that can only be in one state at t time), but I also don't really care. The other question lurking behind all this is that the responsibility of |
… a function that will be called for each node of the tree to decide whether or not it should be included
victorg1991
force-pushed
the
LPS-122662_disabling_nodes_search
branch
from
7fe68af
to
bc067f1
Compare
|
ci:forward |
|
CI is automatically triggering the following test suites:
The pull request will automatically be forwarded to the user
|
✔️ ci:test:sf - 1 out of 1 jobs passed in 3 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPS-122662_disabling_nodes_search 1 Successful Jobs:For more details click here. |
|
Jenkins Build:test-portal-source-format#2103 |
|
All required test suite(s) passed. |
|
Pull request has been successfully forwarded to brianchandotcom#95674 |
|
Jenkins Build:test-portal-acceptance-pullrequest(master)#3890 |
This pr includes:
For testing:
Before:
After:
Thanks!