-
Notifications
You must be signed in to change notification settings - Fork 232
Consistent add storage links #1823
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
app/views/add-config-volume.html
Outdated
@@ -33,7 +33,7 @@ <h2 class="text-center">No config maps or secrets.</h2> | |||
</div> | |||
|
|||
<div ng-if="configMaps.length || secrets.length || ('configmaps' | canI : 'create') || ('secrets' | canI : 'create')" class="mar-top-xl"> | |||
<h1>Add Config Files</h1> | |||
<h1>Add Config Files: {{name}}</h1> |
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.
actually not sure I like this, which editors were we doing this already?
I don't have a really strong opinion about "Add Storage" vs. "Add Storage to..." The question I was asking in my review was, is that overly pedantic? With binding we do call out the thing you're creating the binding for. Not sure about the other editors. Again, no really strong opinion about adding the {{name}}. The drastic change of context is a bigger usability concern, imho. But I also recognize that it's not something with a quick and easy fix. |
That's fair. I guess my point is we don't do that for other actions that work on the deployment config (edit health checks, edit limits, add autoscaler). |
Add storage is the only page where we don't have the name somewhere in the header. @ncameronbritt Inconsistent breadcrumbs from the header in most of these :/ Different header text in these, but they have the name. |
Right. Trying to get some band aid fixes in until we can make the bigger changes. |
So circling back on this: what should we do? I don't have a strong opinion except I'd like the editor to make it more obvious what resource you are editing. Right now the add storage page doesn't say except in the breadcrumbs, which is easy to miss. |
"Add storage to mariadb" would make more sense to me but I'm not strongly opposed to what you have already. |
028a1e7
to
1363ed7
Compare
I updated it to |
LGTM but looks like dist needs updating |
actually looks like the spec tests bombed |
Looks like it might be a flake? |
Removing merge tag until I can rebase. |
- Use "Add Storage" instead of "Add Storage to ..." for deployment configs to be consistent with other pages and other actions like edit health checks and set resource limits - Show the name of the object being edited on the add storage and add config files forms to be consistent with other editors
1363ed7
to
e5c6af9
Compare
[merge][severity: bug] |
Evaluated for origin web console merge up to e5c6af9 |
Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin_web_console/34/) (Base Commit: 6faa37f) (PR Branch Commit: e5c6af9) (Extended Tests: bug) |
configs to be consistent with other pages and other actions like edit
health checks and set resource limits
config files forms to be consistent with other editors
Feedback from @ncameronbritt. Let me know if you agree with these changes.