feat(Popup): add vertical offset #1146
Conversation
|
it's not correct. let me fix. |
Codecov Report
@@ Coverage Diff @@
## master #1146 +/- ##
==========================================
+ Coverage 99.75% 99.75% +<.01%
==========================================
Files 142 142
Lines 2468 2470 +2
==========================================
+ Hits 2462 2464 +2
Misses 6 6
Continue to review full report at Codecov.
|
|
fixed, force pushed |
src/modules/Popup/Popup.js
Outdated
| @@ -76,6 +76,9 @@ export default class Popup extends Component { | |||
| /** Horizontal offset in pixels to be applied to the popup */ | |||
| offset: PropTypes.number, | |||
|
|
|||
| /** Vertical offset in pixels to be applied to the popup */ | |||
| verticalOffset: PropTypes.number, | |||
There was a problem hiding this comment.
How about allowing an X and Y offset in the current offset prop instead of adding a new prop?
offset={[5, 10]}
// vs
offset={5}
verticalOffset={10} We could still allow a single value to apply a horizontal offset only,so it is backwards compatible.
There was a problem hiding this comment.
yea, your api is better. will change tomorrow
|
Looks like this PR is falling behind. Shall we close it, or update and merge? |
…React into vertical-offser # Conflicts: # src/modules/Popup/Popup.js
|
we can close it. |
| @@ -9,7 +9,7 @@ import { default as MessageList } from './MessageList'; | |||
| export interface MessageProps { | |||
| [key: string]: any; | |||
|
|
|||
| /** An element type to render as (string or function). */ | |||
| /** An element type to render as (string or function). */ | |||
There was a problem hiding this comment.
Lint fix. Quite strange issue.
There was a problem hiding this comment.
@levithomason I've updated this PR to latest master and implimented functionality. However, I see there a potential problem, offset tests are testing nothing in fact.
layershifter
added
enhancement
ready for review
and removed
needs author feedback
labels
|
@layershifter Could you expound a bit on "testing nothing"? |
|
@levithomason take a look at the assertions in the |
|
Oh, I see, existing tests were updated. We need new tests that assert this is working. |
|
This has gotten stale and we don't hear much about this feature from the community. Closing for housekeeping. Happy to merge a complete community PR for this feature if submitted. |
Completely inspired from Semantic-Org#1146 and its comments
Completely inspired from Semantic-Org#1146 and its comments
In current implementation it's impossible to change vertical position of popup (without ugly hacks).
This pr changes this.