-
Notifications
You must be signed in to change notification settings - Fork 36
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
Advanced storage options #917
base: main
Are you sure you want to change the base?
Conversation
73120e6
to
424f672
Compare
@anasereijo Let me know your thoughts implementation 🙂 I wasn't sure re: the |
86969e0
to
66bdacd
Compare
5b8ffe0
to
142dc5f
Compare
This comment was marked as resolved.
This comment was marked as resolved.
1dd7264
to
25718c8
Compare
Screen recording of the interaction 🙂 @anasereijo Screencast.From.2025-02-07.14-10-53.webm |
@matthew-hagemann I've been thinking about the use of the 'experimental' chip, and there's no way to avoid wrapping it (especially with translations). that said, I think we should wrap it properly to the next line aligned to the left
everything else is looking good! the interactions are working really nicely 👍 |
@matthew-hagemann actually, I have been thinking about this, and I wonder if it's possible to do something more like this instead: ![]() so the chip/label will be kept always at the top right corner. When the title is too long, wrap it to a second line. |
final target = model | ||
.getAllTargets() | ||
.firstWhereOrNull((t) => t is GuidedStorageTargetReformat); | ||
|
||
if (target == null) return const SizedBox(); |
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.
We will need to generalize this logic here, since we can now see this option while a different guided target (one that is not GuidedStorageTargetReformat
) is selected.
I've tested this with the win10-along-ubuntu
machine config, where we get a Reformat
and two Resize
targets. The latter don't include the CORE_BOOT_PREFER_ENCRYPTED
capability in neither allowed
nor disallowed
, so I think we should hide the option entirely in this case (currently it's shown and enabled, so continuing with the installation will lead to a crash, since we're sending a guided target response that includes an unsupported capability). At the same time we should make sure that the option is always present if it's included in one of those, but keep it disabled if the capability is disallowed.
P.S. I'd appreciate it if we could have some tests that cover those cases:
allowed
:[..., CORE_BOOT_PREFER_ENCRYPTED, ...]
-> tpm option is shown and enabledallowed
:[...]
,disallowed
:[..., CORE_BOOT_PREFER_ENCRYPTED, ...]
-> tpm option is shown but disabled- neither
allowed
nordisallowed
includeCORE_BOOT_PREFER_ENCRYPTED
-> tpm option is not shown
Breaking advanced storage options into its own page. Continuation of #858