sandstone
sandstone copied to clipboard
WRQ-6188: Added FloatingPopup prototype
Checklist
- [x] I have read and understand the contribution guide
- [ ] A CHANGELOG entry is included
- [ ] At least one test case is included for this feature or bug fix
- [x] Documentation was added or is not needed
- [ ] This is an API breaking change
Issue Resolved / Feature Added
There is a requirement for a floating popup capable of free positioning. This should also be able to receive content such as images and text. This is a prototype implementation of the requirements.
Resolution
The component provides a container so that the app can pass contents inside the popup. The component allows you to specify the desired position. In the default sampler, we guide you on how to use it when passing images and text in the sampler.
Additional Considerations
Coordinate conversion may be required as the screen changes. RTL may be required. may need to trigger click events on components that are not hidden by the container.
Links
WRQ-6188
Comments
Enact-DCO-1.0-Signed-off-by: Hyelyn Kim ([email protected])
Codecov Report
Attention: 4 lines
in your changes are missing coverage. Please review.
Comparison is base (
6cf0adf
) 81.24% compared to head (4dde49b
) 81.27%. Report is 10 commits behind head on develop.
Files | Patch % | Lines |
---|---|---|
PopupContainer/PopupContainer.js | 87.50% | 4 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## develop #1557 +/- ##
===========================================
+ Coverage 81.24% 81.27% +0.03%
===========================================
Files 141 142 +1
Lines 6499 6531 +32
Branches 1921 1928 +7
===========================================
+ Hits 5280 5308 +28
- Misses 932 936 +4
Partials 287 287
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
I have some questions about the props and component naming.
- Popup component already renders in a FloatingLayer. FloatingPopup sounds confusing to me. What do you think?
- Don't we need a prop for supporting on/off RTL positioning? When I change the locale to the ar-SA, icon and text flips which makes sense but the position remains. Some cases may remain but some cases may change the position to the right-based align. What do you think?
@seunghoh
I have some questions about the props and component naming.
- Popup component already renders in a FloatingLayer. FloatingPopup sounds confusing to me. What do you think?
- Don't we need a prop for supporting on/off RTL positioning? When I change the locale to the ar-SA, icon and text flips which makes sense but the position remains. Some cases may remain but some cases may change the position to the right-based align. What do you think?
-
I agree. So I changed naming to PopupContainer from FloatingPopup. This means that it is a container with the function of creating a popup.
-
Good. I added a prop for supporting on/off RTL position. But I should check whether it is needed under the normal circumstances of the app through the guide. If necessary, i should check whether the app wants to change only the location or the content direction as well.
LGTM. We don't have a fixed requirement yet, so I will keep this as a draft PR.