breaking(popup): verticalOffset

[adam-26]

Breaking Change

Popup must rename the offset property to horizontalOffset

Before

<Popup offset={10} />

After

<Popup horizontalOffset={10} />

---

Add a verticalOffset prop to the popup component.

Fixes #2449.

[codecov-io]

Codecov Report

Merging #2450 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2450      +/-   ##
==========================================
+ Coverage   99.73%   99.74%   +<.01%     
==========================================
  Files         154      154              
  Lines        2692     2696       +4     
==========================================
+ Hits         2685     2689       +4     
  Misses          7        7

Impacted Files	Coverage Δ
src/modules/Popup/Popup.js	100% <100%> (ø)	⬆️

---

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 d077a17...fa2832c. Read the comment docs.

[levithomason]

Great, thanks for this! We had this same PR submitted some time ago and never finished.

In an effort to keep the API consistent, let's consider a prop rename/restructure here. Currently, we have offset for horizontal values and verticalOffset for vertical values. Preferably, these would be more consistent.

Option 1

Rename offset to horizontalOffset. Downside, it is a breaking change. Upside, it is explicit, consistent, and fits the SUI tone.

<Popup horizontalOffset={} verticalOffset={} />

Option 2

Allow offset to accept an object. Downside, it is a little magical and nested prop values aren't ideal. Upside, it is concise, maps to DOM element position values, and is not a breaking change.

<Popup offset={10} />                // horizontal
<Popup offset={{ x: 10, y: 20 }} />  // horizontal, vertical

---

Thoughts?

[levithomason]

Heads up, this is actually a duplicate of #2444. I'll let the PR authors decide on which PR to focus efforts.

[adam-26]

@levithomason, I think option 1 fits best with SUI. Why not just add ‘horizontalOffset’ as an alias to offset? This way, it’s not a breaking change. You could add a warning about using ‘offset’ and deprecate it in a future version?

[levithomason]

Option 1 it is 👍. We don't support backward compatibility while in development (0.x) so no need to alias.

[levithomason]

Our changelog is auto-linked to PRs. So that users can easily identify breaking changes and upgrade accordingly, we include a very brief description of why we are breaking the API and a before/after snippet. Example PR here #1791.

Let's update the description is this PR as well.

[adam-26]

@levithomason please review changes, and that the PR description is correctly formatted for the change log.

This also resolves #2444.

[levithomason]

The fourth offset example, vertical 50 from bottom center, seems to misbehave. The Popup appears then disappears immediately afterward. This is the snippet:

<Popup
  trigger={<Icon size='large' name='heart' circular />}
  content='As expected this popup is way off to the bottom'
  verticalOffset={50}
  position='bottom center'
/>

However, when changed to -50, it behaves correctly and is positioned far below as it should be. Seems there is a bug there.

[adam-26]

@levithomason bug is now fixed and tested. Please review again.

[levithomason]

Awesome, this looks great. Thanks much!

[levithomason]

Released in semantic-ui-react@v0.78.0.