Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WRQ-6188: Added FloatingPopup prototype #1557

Draft
wants to merge 8 commits into
base: develop
Choose a base branch
from
Draft

Conversation

mmyelyn
Copy link
Contributor

@mmyelyn mmyelyn commented Feb 6, 2024

Checklist

  • 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
  • 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 (myelyn.kim@lge.com)

- added floating popup component prototype
- added floating popup default sampler

Enact-DCO-1.0-Signed-off-by: Hyelyn Kim (myelyn.kim@lge.com)
Copy link

codecov bot commented Feb 6, 2024

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 ⚠️
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              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mmyelyn mmyelyn marked this pull request as draft February 7, 2024 06:04
- added unit tests
- cleaned up floating popup

Enact-DCO-1.0-Signed-off-by: Hyelyn Kim (myelyn.kim@lge.com)
@seunghoh
Copy link
Member

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?

- Changed component name to clearly
- Added rtl prop and implemented rtl position

Enact-DCO-1.0-Signed-off-by: Hyelyn Kim (myelyn.kim@lge.com)
Enact-DCO-1.0-Signed-off-by: Hyelyn Kim (myelyn.kim@lge.com)
PopupContainer/PopupContainer.js Outdated Show resolved Hide resolved
PopupContainer/PopupContainer.js Outdated Show resolved Hide resolved
PopupContainer/PopupContainer.js Outdated Show resolved Hide resolved
PopupContainer/PopupContainer.js Show resolved Hide resolved
PopupContainer/PopupContainer.js Show resolved Hide resolved
PopupContainer/PopupContainer.js Outdated Show resolved Hide resolved
PopupContainer/PopupContainer.js Outdated Show resolved Hide resolved
PopupContainer/PopupContainer.js Outdated Show resolved Hide resolved
PopupContainer/PopupContainer.module.less Outdated Show resolved Hide resolved
- Modified property name
- Modified so that the location changes unconditionally depending on the direction
- Fixed lint warning

Enact-DCO-1.0-Signed-off-by: Hyelyn Kim (myelyn.kim@lge.com)
@mmyelyn
Copy link
Contributor Author

mmyelyn commented Feb 19, 2024

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

Enact-DCO-1.0-Signed-off-by: Hyelyn Kim (myelyn.kim@lge.com)
Enact-DCO-1.0-Signed-off-by: Hyelyn Kim (myelyn.kim@lge.com)
Enact-DCO-1.0-Signed-off-by: Hyelyn Kim (myelyn.kim@lge.com)
@SkylerBaek
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants