-
Notifications
You must be signed in to change notification settings - Fork 121
Make privacy banner not dismissable via WordPress UI tooling #9830
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
Conversation
You can test the changes from this Pull Request by:
|
| // We define them here instead than in the existing extension that defines the protocol conformance so that we can make the stored constants. | ||
| // Otherwise, since extensions don't allow for stored properties, they would have had to be computed vars. | ||
| let allowsTapToDismiss = false | ||
| let allowsDragToDismiss = false |
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.
I think we will also need let allowsUserTransition = false to prevent all gestures from dismissing the app.
[Q]: What problem do you see in making these computed vars? I think it may be easier for future maintainers to find all of these properties under the same protocol extension, what do you think? 😅
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.
Discoverability is a legit issue 🤔
What problem do you see in making these computed vars?
These values don't change, it seems inefficient to compute them at runtime when they could be set once.
Having said that, if it helps with discoverability, I'm happy to move them in the extension.
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.
Addressed in 5fc8207
Generated by 🚫 dangerJS |
Ecarrion
left a comment
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.
Looks and tests great!
Thanks @mokagio for tackling this change!
5fc8207 to
88ed395
Compare
Description
Removes the workaround that was required while waiting for the changes in wordpress-mobile/WordPressUI-iOS#126 to be published
Testing instructions
I tested this by forcing the app to present the banner and by both tapping out of it and trying to drag it (in the Simulator) to verify it staid on screen. I'm not familiar with how the component should behave, and we don't have a demo for it in WordPress-UI, so I might have missed something.
I recorded myself during the process, but QuickTime crashed when I tried to crop the video...
For reference, here's the diff I used to force the display:
Diff
RELEASE-NOTES.txtif necessary.