-
-
Notifications
You must be signed in to change notification settings - Fork 51
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
Replace privacy policy page. #79
Conversation
Relative anchors don't work right now in the So that means the sidebar with the table of content probably won't be added for now. I'm also considering to leave out the table of content inside the text as it, well... won't work. Will probably be more confusing if we left it in. |
c43dceb
to
86b3110
Compare
Visit the preview URL for this PR (updated for commit 661ddd7): https://sharezone-test--pr79-privacy-policy-rewor-4w6bbtfu.web.app (expires Wed, 27 Jul 2022 16:08:50 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
Flexible( | ||
child: ConstrainedBox( | ||
constraints: BoxConstraints(maxWidth: 600), | ||
child: Padding( | ||
padding: const EdgeInsets.only(top: 20), | ||
child: Column( | ||
mainAxisAlignment: MainAxisAlignment.center, | ||
crossAxisAlignment: CrossAxisAlignment.center, | ||
children: const [ | ||
_PrivacyPolicyHeading(), | ||
Padding( | ||
padding: const EdgeInsets.symmetric( | ||
horizontal: 20, vertical: 8.0), | ||
child: _PrivacyPolicySubheading(), | ||
), | ||
Divider(), | ||
Flexible(child: _PrivacyPolicyMarkdown()), | ||
Divider(), | ||
Padding( | ||
padding: | ||
const EdgeInsets.symmetric(vertical: 20), | ||
child: _AcceptionButtons(), | ||
) | ||
], | ||
), | ||
), | ||
), | ||
), | ||
], |
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.
@nilsreichardt
I want to constrain the privacy policy text to a maxWidth (wich works via ConstrainedBox
) but I want to center the text (which does not work).
Wrapping the Flexible
in a Center
or Alignement
does not work. Changing the mainAxisAlignement
of the Row
which wrapps these widgets isn't a soluton (i think), since I want to lay out the table of contents at the left and only center the text.
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.
You need to wrap the ConstrainedBox
into a Expanded
widget. Therefore, takes the widget the complete remaining space of the parent Row
(_TableOfContents
and VerticalDivider
takes only what the need). Now to center the ConstrainedBox
inside the Expanded
widget, you need to wrap it with a Center
widget.
Row(
children: [
_TableOfContents(),
VerticalDivider(),
Expanded(
child: Center(
child: ConstrainedBox(
constraints: BoxConstraints(maxWidth: 600),
child: Padding(
padding: const EdgeInsets.only(top: 20),
child: Column(
mainAxisAlignment: MainAxisAlignment.center,
crossAxisAlignment: CrossAxisAlignment.center,
children: const [
_PrivacyPolicyHeading(),
Padding(
padding: const EdgeInsets.symmetric(
horizontal: 20, vertical: 8.0),
child: _PrivacyPolicySubheading(),
),
Divider(),
Flexible(child: _PrivacyPolicyMarkdown()),
Divider(),
Padding(
padding: const EdgeInsets.symmetric(
vertical: 20),
child: _AcceptionButtons(),
)
],
),
),
),
),
),
],
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.
Wtf, I'm sure I tried this exact combination before and for me if I used Expanded
my BoxConstraints
were just ignored 🤔🤔
Would be really helpful if I could build for macOS since Web doesn't support Hot reload. Currently I'm running everything in a iPad Simulator on my MacBook since it's close to a desktop regarding the form factor. I can't really resize it though - I can only scale it (which doesn't affect the app since the scaling only changes the size of the iPad on my MacBook, not the size that is used for rendering on the iPad). @nilsreichardt #64 is blocked on #153, correct? Can I help there somehow? |
Yes, this is correct. The new Flutter version broke our theme and this need to be fixed. Maybe I can fix it in today in evening or you will do it before |
@nilsreichardt Ready for review! :D The only thing I'll still have to do is to open issues for some of the stuff that's written in the PR (some of the unchecked checkboxes). Good luck haha |
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.
LGTM 🎉
Awesome PR 🚀🚀🚀
It's too big to make really a review. If you think, everything is fine. You can merge it 👍
Maybe it would look better to also center the "Weitere Optionen" since the button are also centered. Or setting the alignment of the button to left. But I think mixing it looks a bit weird.
app/test/privacy_policy/table_of_contents/toc_currently_reading_test.dart
Show resolved
Hide resolved
app/test/privacy_policy/table_of_contents/toc_section_expansion_test.dart
Show resolved
Hide resolved
app/test/privacy_policy/table_of_contents/toc_section_expansion_test.dart
Show resolved
Hide resolved
app/test/privacy_policy/table_of_contents/toc_section_expansion_test.dart
Show resolved
Hide resolved
app/test/privacy_policy/table_of_contents/toc_section_expansion_test.dart
Show resolved
Hide resolved
app/test/privacy_policy/table_of_contents/toc_section_expansion_test.dart
Show resolved
Hide resolved
app/test/privacy_policy/table_of_contents/toc_section_expansion_test.dart
Show resolved
Hide resolved
Btw: Would be really nice to have something like this as a package. Even as an internal package would be nice |
Yes, of course, I changed that 👍 |
Co-authored-by: Nils Reichardt <[email protected]>
Co-authored-by: Nils Reichardt <[email protected]>
Will do that in another PR |
Replace old privacy policy pages with a new markdown-based, multi-layout privacy policy page.
This PR will not update the privacy policy itself, only the presentation.
Note: This PR originally had code for loading the privacy policy asynchronously (it wasn't loaded from the internet, but all the code for loading indicators, errors, etc. was 90% there). I removed it for now, to have less complex code as we don't need it yet. The deleted code: 7eb67fb3fd4694e1cf4505cc12df821198ea7f22
Desktop / Narrow layout
(Updated: "Weitere Optionen" in the lower-left is now centered)
privacy_policy_x264_30.mp4
Mobile layout
Privacy policy with Subchapters (just for demonstration purposes)
2.Sharezone.Web-App.-.Google.Chrome.2023-05-08.20-12-55.mp4
Tidbits to remeber:
TODOs:
markdown
plugin is different from the Id we use in the text. Seems to happen for Anchors with Sonderzeichen/Umlaute?flutter: 'package:sharezone/onboarding/sign_up/pages/privacy_policy/src/table_of_contents/currently_reading.dart': Failed assertion: line 104 pos 14: 'oldVisibleHeadings.length == 1': is not true.
Theme.visualDensity
- should be additionally look at the type of input used? See also: https://stackoverflow.com/questions/63327808/in-flutter-how-can-i-check-if-a-mouse-device-or-a-touch-deviceI changed the implementation but didn't change the design.
ui_privacy_policy_text_scaling_factor_changed
14. Instance ID
subheadings (e.g.Firebase Cloud Messaging
) have to much space to the left ( subheadings of e.g.8. Account, Nickname und Passwort
are more on the left, like they should)common.dart
into files.GrayShimmer
?). Disable buttons while loading.- If changes are made in the privacy policy display code
- If a new privacy policy is written
(Should check all privacy policies with all versions of the privacy policy display code)
?
symbol to the right of the wordNeed Feedback on:
Nice to have but not worth it right now (add as issues?):
Stuff that doesn't work 100% as I want it but I can't or don't want to fix: