Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

feat(Popup): autocontrolled mode #319

Merged
merged 3 commits into from
Oct 6, 2018
Merged

Conversation

kuzhelov
Copy link
Contributor

@kuzhelov kuzhelov commented Oct 5, 2018

TODO

  • update CHANGELOG

This PR introduces uncontrolled mode to the Popup, so now it is possible to consume it in the following way for the simplest scenarios:

<Popup content='Popped up!'>
  <Button>Click to open the popup</Button>
</Popup>

FAQ

  • In Uncontrolled mode, we have Popup reacting on click trigger's event only - is it usual the popup to be open always on click?

No, it is not - and to parametrise this behavioral aspect, we should consider to introduce additional prop (like triggerAction) to the API of Popup:

// where click, presumably, would be default value
<Popup triggerAction='click' content=..>
   <Button />
</Popup>

Also, this will introduce another benefit to us. Now related reasonable question could be asked - is it correct that we have a PopupBehavior as a default one (note that PopupBehavior's implementation now is tightly coupled with the click trigger concept - and, thus, should probably be renamed to something that will contain 'click' in its name!). Surely, no. But the reason it is introduce as a default one for now is that it covers the only uncontrolled case we have now. At the same time, with triggerAction prop introduced it would be possible to set accessibility behavior aspects for the Popup.

See this as a step that should be preempted by the set of careful investigations and experiments - and, thus, a separate dedicated PR should be introduced for that.

@codecov
Copy link

codecov bot commented Oct 5, 2018

Codecov Report

Merging #319 into master will decrease coverage by 0.21%.
The diff coverage is 7.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #319      +/-   ##
==========================================
- Coverage   89.54%   89.32%   -0.22%     
==========================================
  Files          62       62              
  Lines        1176     1180       +4     
  Branches      152      176      +24     
==========================================
+ Hits         1053     1054       +1     
- Misses        121      124       +3     
  Partials        2        2
Impacted Files Coverage Δ
src/components/Popup/Popup.tsx 41.37% <7.69%> (-1.22%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d5a3da...2ef828f. Read the comment docs.

@mnajdova
Copy link
Contributor

mnajdova commented Oct 5, 2018

I like the idea for introducing the triggerAction. We should investigate more about this.

@kuzhelov kuzhelov merged commit aa09709 into master Oct 6, 2018
@layershifter layershifter deleted the feat/autocontrolled-popup branch November 9, 2018 09:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants