-
Notifications
You must be signed in to change notification settings - Fork 74
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
Show volume's usage in "Add disk" dialog as a Form helper text #756
Conversation
Alternatively we also discussed showing this as an error or a text variant. As there are use cases (e.g. disk sharing) where user might want to attach a VM used by another VM, therefore I think it should not be an error. I gave it some thought and I agree with what was mentioned on standup, that it's probably the best fitting as a warning variant, as disk sharing is quite a corner case. |
Please rebase for the packit tests, the timeout is now 60 minutes. |
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.
Thanks! That's better.
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.
Ah, wait. Why isn't the select showing an underline and icon?
https://www.patternfly.org/v4/components/form/design-guidelines/#errors-and-validation
I think it's supposed to look like this:
https://www.patternfly.org/v4/components/select/html#warning
For React, set validated
to warning
.
Tests getting quite green, @martinpitt @jelly @marusak Mind giving it a code 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.
@garrett FYI There is one more situation where the warning contains 2 sentences. If the disk is raw, it's being automatically transfprmed to "shareable" disk, and the warning looks like this: For non-raw disk, sharing the disk is not possible, therefore the other sentence is not shown. But you can still use the same disk for 2 vms, you will just not be able to run them at the same time (libvirt will not start the second VM), so I think it's still a warning and not an error. |
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.
Dakujem!
src/components/vm/disks/diskAdd.jsx
Outdated
let text = _("This volume is already used by ") + vmsUsing; | ||
const volume = storagePool.volumes.find(vol => vol.name === volumeName); | ||
if (volume.format === "raw") | ||
text += ". " + _("Attaching it will make this disk shareable for every VM using it."); |
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.
This kind of addition may also be problematic. It's not a given that every language uses a period. Honestly, I'd rather compromise a little and always add the period to the "already used by ..." message. Otherwise you need two string templates which just differ by ".", and that's a little rude towards translators. But this doesn't map grammar-wise very well. E.g. in German you would always end a sentence with a period. (Honestly it's rather confusing that English doesn't? or is that just some PF guideline? If so, it's very English centric.)
Alternatively, force a line break and drop the period from both sentences? Then there's at least some consistency in the UI.
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 like the line break idea. We use something similar for snapshots:
WDYT @garrett
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.
On the second thought, it seems that pattenrfly forces one-liner text for Form helperText by ignoring \n
. We could do some workaround, but maybe it's just simpler to use sentences with dots on the same line.
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.
+1 on just using proper sentences, yes.
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.
Reverted back to using sentences ending with a comma. But let's wait if @garrett agrees.
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 didn't wait before merging.
What does "Attaching it will make this disk shareable for every VM using it" mean? 🤔 |
It will make it "Writeable and shareable" for every VM using it (redhat docs) The whole discussion why we do this can be found here |
Instead of an inline dialog, an information about volume being used a disk by another VM should be shown as a form helper text. Fixes cockpit-project#754
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.
Dakujem!
Oh, this is another one that was prematurely merged. As I pointed out, the string "Attaching it will make this disk shareable for every VM using it" doesn't make sense. I couldn't suggest anything else as I didn't know what it meant (nor would our users), so I asked what it meant. I still don't know:
This needs an immediate fixup to land before the release or it should be reverted. If this is actually a bad thing that will cause data loss, then this needs to be a (!) danger not a <!> warning. Otherwise, suggestion: "Adding this disk will change its mode to writable and shared." |
Additionally, "Previously taken snapshots allow you to revert to an earlier state if something goes wrong"?!?!? But there are no snapshots that have been previously taken. It's an empty state! This also needs rewording. Suggestion: "Snapshots are used to revert a VM to a earlier state" ...although it's already been a long day and it's hot, so I'm second-guessing my brain. Regardless, I think these two suggestions (this and the one in the comment above) are improvements. We can talk about it tomorrow if you have improvements on them. |
This PR does not touch snapshots, so this needs to be changed independently. I'll split it out into an issue. |
This also already existed before, so should be changed independently. Split out as issue #765. |
Instead of an inline dialog, an information about volume being used a
disk by another VM should be shown as a form helper text.
Fixes #754