New: Add pseudos that depend on optional Adapter APIs
#139
Conversation
| /** | ||
| * is the element in hovered state? | ||
| */ | ||
| isHovered?: (elem: ElementNode) => boolean; |
There was a problem hiding this comment.
Should I name it as getHovered or isHovered is fine?
This method checks if give elem is in hovered state or not. So I named it as isHovered.
Instead if we could have used getState and then check state === 'hovered', then get suits more.
There was a problem hiding this comment.
I'm okay with these — just having a subset available seems alright.
lib/pseudos.js
Outdated
| @@ -189,6 +189,13 @@ function getFirstElement(elems, adapter) { | |||
| } | |||
| } | |||
|
|
|||
| //pseudos that depends on optional Adapter methods. (key, value) = (pseudoName, adapterMethod) | |||
| var pseudosWithOptionalImplementation = { | |||
There was a problem hiding this comment.
I kept this list, so that before getting function for particular pseudo, we can check if its associated Adapter method is implemented or not. Otherwise we will simply use falseFunc.
lib/pseudos.js
Outdated
| @@ -262,7 +269,24 @@ var pseudos = { | |||
| link: function(elem, adapter) { | |||
| return adapter.hasAttrib(elem, "href"); | |||
| }, | |||
| visited: falseFunc, //Valid implementation | |||
| hover: function(elem, adapter) { | |||
| if (adapter.isHovered) { | |||
There was a problem hiding this comment.
I am checking this here also in case pseudo function is being used directly instead of from compile.
As we are exporting all pseudos, third party can use this function directly also.
There was a problem hiding this comment.
My preference would be to implement these as filters, which can return falseFunc themselves.
|
Hi @fb55 , could you please review the current code, and clear my doubts so that I can proceed on this PR. |
| /** | ||
| * is the element in hovered state? | ||
| */ | ||
| isHovered?: (elem: ElementNode) => boolean; |
There was a problem hiding this comment.
I'm okay with these — just having a subset available seems alright.
lib/pseudos.js
Outdated
| @@ -262,7 +269,24 @@ var pseudos = { | |||
| link: function(elem, adapter) { | |||
| return adapter.hasAttrib(elem, "href"); | |||
| }, | |||
| visited: falseFunc, //Valid implementation | |||
| hover: function(elem, adapter) { | |||
| if (adapter.isHovered) { | |||
There was a problem hiding this comment.
My preference would be to implement these as filters, which can return falseFunc themselves.
jaspreet57
force-pushed
the
feat/element-state-support
branch
from
f36d6e2
to
ed044ff
Compare
|
@fb55 Thanks for the feedback. I have added these pseudos as filters. Could you help me in test cases. Here test cases seems to depend on the implementation of default Adapter APIs. So, to get full coverage of new pseudos, we somehow have to provide the implementation of optional Adapter methods. One way is to pass my own custom Adapter. Another way could be to mock something else. Thoughts please ! |
|
Given that the current behavior is now covered by a test, I'm okay with merging as-is in order of not blocking this any further. Thanks a lot @jaspreet57! |
Pseudos like element state
hover,active,visited, etc can be supported conditionally.This PR enables such pseudos to be supported only if associated Adapter method is implemented.
For example:
hoverpseudo may give results, only ifisHoveredis implemented in given Adapter APIs. Otherwise,hoverwill result intofalseFuncand will give empty result.This logic can be improved in various ways. Please review once and see my comments on code and let me know if I am going in right direction.
I will be updating
Readmeand Test Cases once logic is finalised.Resolves: #80