-
Notifications
You must be signed in to change notification settings - Fork 0
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
add a feature for dropdown to stretch out to fullscreen on mobile #65
Conversation
Reviewer's Guide by SourceryThis PR implements a fullscreen dropdown feature for mobile views and refactors the dropdown's sizing behavior. The implementation moves styling control to CSS parts, introduces a new mobile breakpoint feature, and restructures the component's DOM manipulation for better performance. Class Diagram for AuroDropdown and AuroFloatingUIclassDiagram
class AuroDropdown {
+String mobileFullscreenBreakpoint
+void firstUpdated()
+void notifyReady()
}
class AuroFloatingUI {
+void mirrorSize(Boolean fullscreen)
+void position()
+void handleMobileFullscreen(Boolean isMobile)
+void updateState()
+void configure(Element elem)
}
AuroDropdown --> AuroFloatingUI : uses
note for AuroDropdown "Added mobileFullscreenBreakpoint attribute"
note for AuroFloatingUI "Added methods for handling fullscreen mode"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
5ceda97
to
7222ccc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @sun-mota - I've reviewed your changes - here's some feedback:
Overall Comments:
- There are inconsistencies in the documentation - 'mobileFullscreenBreakpoint' vs 'fullscreenOnMobile'. Please standardize on one attribute name throughout the documentation.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
7222ccc
to
80f5af8
Compare
91afade
to
32557f1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to change up how the new attribute is used a bit. I'll schedule some time to pair with you on this.
32557f1
to
126a643
Compare
@sourcery-ai review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @sun-mota - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding test coverage for the new mobile fullscreen functionality, particularly around breakpoint handling and size transitions
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -16,6 +16,13 @@ | |||
position: relative; | |||
} | |||
|
|||
#bibSizer { | |||
position: absolute; | |||
z-index: -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this? It's not an element that you can focus, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact... do we need the pointer events rule either?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
z-index:-1
will help the performance by putting the layer to be the most back so that other layer would get any cursor events first. pointer-events
rule would also help the performance for the browser to skip(not remove from the interaction queue) the cursor events on this layer.
IMO, I think adding these ruler wont hurt and rather help a little bit.
f90f3a5
to
1f4e69f
Compare
…o improve code organization
1f4e69f
to
faa6005
Compare
🎉 This PR is included in version 1.6.0-beta.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Alaska Airlines Pull Request
Width and Sizing Behavior
Width: The
auro-dropdown
component will automatically consume the full width of its parent container. To make it narrower, you can style thebibSizer
part.Styling Options: Only the following styles can be applied to the
bib
part:width
height
maxWidth
maxHeight
Fullscreen Effect on Mobile View
On mobile view, adding the
mobileFullscreenBreakpoint="{breakpoint-token}"
will switch the dropdown to fullscreen mode.By submitting this Pull Request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I have performed a self-review of my own update.
Summary by Sourcery
Add a feature to control dropdown fullscreen behavior on mobile devices using a breakpoint attribute, enhance dropdown styling options, and update documentation accordingly.
New Features:
Enhancements:
Build:
Documentation:
Summary by Sourcery
Add a feature to control dropdown fullscreen behavior on mobile devices using a breakpoint attribute, enhance dropdown styling options, and update documentation accordingly.
New Features:
Enhancements:
Build:
Documentation: