Jl - ui for protocol merge #1721
Conversation
|
@marklohr Ah, good catches thanks! |
| @@ -0,0 +1,19 @@ | |||
| #protocol-merges-container | |||
| %h1{style: 'text-align:center;'} | |||
There was a problem hiding this comment.
Replace inline style with the text-center class
| #protocol-merges-container | ||
| %h1{style: 'text-align:center;'} | ||
| Protocol Merge Tool | ||
| %p   |
There was a problem hiding this comment.
Standard tags shouldn't really be used for spacing like this. You could use breaks or add margin styles
| %p   | ||
| %p   | ||
| %p   | ||
| %p |
There was a problem hiding this comment.
All of this content should be wrapped in a form tag with the URL being the perform_protocol_merge route that you added. Also can you add the bootstrap form style classes for consistency? https://bootstrapdocs.com/v3.3.5/docs/css/#forms
| %p   | ||
| %p   | ||
| %p{style: 'text-align:center;'} | ||
| %button.btn.btn-info.navbar-btn#merge-button{ type: 'button', title: "Perform Protocol Merge"} |
There was a problem hiding this comment.
This button should be a submit_tag for the form. To force the user to confirm before submitting, you can use the onsubmit attribute. https://stackoverflow.com/a/6515632
| @@ -35,6 +35,8 @@ | |||
| %li | |||
| %button.btn.btn-info.navbar-btn#epic-queue-btn{ type: 'button', title: t(:dashboard)[:navbar][:tooltips][:epic_queue], data: { toggle: 'tooltip', placement: 'bottom', delay: '{"show":"500"}'} } | |||
| = t(:dashboard)[:navbar][:epic_queue] | |||
| - if current_user.catalog_overlord? | |||
| = link_to "Protocol Merge", dashboard_protocol_merge_path, :class => ' header_button btn btn-primary btn-md', style: 'margin-top:7px;' | |||
There was a problem hiding this comment.
This text needs to be YAMLized and styles moved to a stylesheet
jleonardw9
dismissed
kayla-glick’s stale review
This is really great feedback, and I plan on implementing these and many other changes in future iterations. This is just a first draft so a select group of users (just Leila, Wenjun, etc.) can test and make sure the merge works like expected. Only a couple of people will even see this tool.
https://www.pivotaltracker.com/n/projects/1918597/stories/155628270