-
Notifications
You must be signed in to change notification settings - Fork 472
Conversation
validates :public, inclusion: { in: [true] }, if: :global? | ||
enum visibility: [:visibility_private, :visibility_protected, :visibility_public] | ||
|
||
validate :global_namespace_cannot_be_private |
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.
Unfortunately, the following code (and some variations) didn't work:
validates :visibility, inclusion: { in: [:visibility_public, :visibility_protected] }, if: :global?
Using a separate function works perfectly though.
It generally looks good to me, so thanks for taking care of this. That being said, I really don't like the UI for two things:
|
Thanks for reviewing and your feedback 👍 I don't entirely agree with your dislike of the UI though. How exactly is it visually confusing? The current visibility is indicated by the primary button, the others are default, i.e. not selected. Should the icons be confusing, which I hope their not, the tooltip will help the user understand. For namespaces the user cannot change the visibility of, the buttons are disabled and therefore non-clickable. Regarding the alert, I just saw that there was one previously with the toggle button which I had forgotten to carry over. However, is it really necessary? The user will see that there's a different primary button when changing the visibility. Maybe, in case of error ( I wouldn't know what could go wrong here since it's basically just a 3-states toggle button), there could be an alert. I partly agree with you on the space thing. It sometimes can be a bit too long and therefore wrap, which isn't pretty. We could increase the column width though, then it (hopefully) wouldn't wrap that fast. I'm not entirely sure though whether or not a select element would fix this. Any other opinions and suggestions? |
An "internal" (or "protected") namespace allows only logged-in users to use the repositories belonging to it. Other than that, it behaves just like a public namespace. Resolves #606 Signed-off-by: Thomas Hipp <[email protected]>
Whenever you have multiple options and a selected option, a select element is always better in my opinion (unless there are tons of available options of course). It is more compact and users know the meaning of it. For example, when I entered for the first time, there was no selected button (probably a bug ?). So, as a new user, without touching anything, I may wonder, what are these things ? Maybe they are info boxes ? (like the ones we have since #892). A select element simply removes this confusion.
That's a good point, but it all boils down to feedback. If we were not using JS, this wouldn't even be needed, because with a refresh users are sure that their request hit the server. However, since we are using JS, one may wonder: what about my request ? Did it succeed ? Does the server know ? Honestly, when I visit a web site with this issue, I usually reload the page just to make sure that the thing persisted (which totally defeats the purpose of JS). So, it might seem redundant, but imho it's good for visual feedback. |
I agree that users will know the meaning of a select element, but it shouldn't really matter if we only have 3 options. As you said, for a long list a select element makes perfect sense.
I cannot reproduce this bug. Also, I think (new) users can distinguish these different types of buttons. If not, then we could just change the tooltips to something like "change visibility to protected."
If the request succeeds, the button will change. If it fails (again, this should not actually happen), the button stays the same. This is pretty simple and also a good feedback. However, if users want this feedback, it's fine with me. |
@cyntss will take another look at Portus before we release 2.1. So, we can delay this discussion until then 😉 |
An "internal" (or "protected") namespace allows only logged-in users to
use the repositories belonging to it. Other than that, it behaves just
like a public namespace.
Resolves #606
Signed-off-by: Thomas Hipp [email protected]