sandstone icon indicating copy to clipboard operation
sandstone copied to clipboard

WRQ-6188: Added FloatingPopup prototype

Open mmyelyn opened this issue 1 year ago • 4 comments

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])

mmyelyn avatar Feb 06 '24 11:02 mmyelyn

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.

codecov[bot] avatar Feb 06 '24 11:02 codecov[bot]

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 avatar Feb 15 '24 01:02 seunghoh

@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?
  1. I agree. So I changed naming to PopupContainer from FloatingPopup. This means that it is a container with the function of creating a popup.

  2. 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.

mmyelyn avatar Feb 19 '24 06:02 mmyelyn

LGTM. We don't have a fixed requirement yet, so I will keep this as a draft PR.

0x64 avatar Feb 20 '24 08:02 0x64