Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

Add Requested By to Imaging #437

Merged
merged 4 commits into from
May 13, 2016

Conversation

clettenberg
Copy link
Contributor

@clettenberg clettenberg commented Apr 29, 2016

Fixes #362.

Changes proposed in this pull request:

  • Adds Requested By to Imaging Requests and Completed lists
  • Refactors Imaging to use components in blocks
  • Adds Add Note link to the Requests list items that don't have notes
  • Adds No Notes and No Results to Completed list items that don't have either Notes or Results

Add Notes link:
link

No Notes and No Results
light
cc @HospitalRun/core-maintainers

@jkleinsc
Copy link
Member

@cacqw7 thanks for the PR!
A couple of things:

  1. Can you change your code to use localization for the text displayed? More details about localization using ember-i18n can be found here: https://github.com/jamesarosen/ember-i18n/wiki/Doc:-Translating-Text
  2. Can you provide screenshots of the change you made.

@clettenberg
Copy link
Contributor Author

@jkleinsc done and done!

@jkleinsc
Copy link
Member

jkleinsc commented May 2, 2016

@cacqw7 thanks for the update!

@jglovier Any objections/comments on the UI changes for this PR?

@jglovier
Copy link
Member

jglovier commented May 3, 2016

  • Right now there is a lot of wrapping of very short fields. It'd be better if we could shorten up the Notes field (maybe max-width: 300px?) and give space for the other fields to not wrap so easily.

@jglovier
Copy link
Member

jglovier commented May 3, 2016

(Sorry, entered that last comment before finished...)

  • I'm not a fan of the "No results" and "No notes" placeholder text because it's redundant and unnecessary, and adds visual noise. If there are no notes to display, I think the absence of them is clue enough to that.
  • I like surfacing the Add notes link, but I'm curious to know more of your intentions there. Does it only appear on hover, or visible by default on every item without notes? I'd be opposed to adding it on every single item w/o notes for the same visual noise concerns as above, even if it is more useful

Unfortunately some of this just comes back to incomplete work on how actions are surfaced on items in the first place. I don't want to block too much here on account of that, but those are important concerns to resolve before we go too far with implementing more pieces that will be addressed by that.

Thanks for jumping in here @cacqw7! ✨

@clettenberg
Copy link
Contributor Author

@jglovier

I'll go ahead and remove No Results, No Notes and the Add Notes. Those were all outside the scope of #362 and I agree that there are more important things to resolve first.

I'll work on the margins this weekend and push up changes/screenshots 👍 😄

@jglovier
Copy link
Member

jglovier commented May 5, 2016

@cacqw7 awesome. 🤘 As we get closer to resolving the surfacing actions in index views piece, I'll rope you back in and we can revisit these pieces under the new paradigm.

@clettenberg clettenberg force-pushed the add-requested-by-to-imaging branch from a21a1ef to e3bdc6f Compare May 7, 2016 13:08
@clettenberg
Copy link
Contributor Author

clettenberg commented May 7, 2016

@jglovier I added the min-width you suggested.

request

min-width

I also added Requested By to the Edit screen

edit

@clettenberg clettenberg force-pushed the add-requested-by-to-imaging branch from e3bdc6f to 95d53ee Compare May 7, 2016 23:10
@@ -0,0 +1,2 @@
.imaging-row .notes { max-width: 300px; }
.completed-imaging-row .notes { max-width: 300px; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cacqw7 not sure why we need two different classes here. @jglovier any opinion on using two different classes for completed vs in-progress?

Copy link
Member

@jglovier jglovier May 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, if the value is the same (which it appears to be) and there is no other style conflict, just use the existing classname.

@jglovier
Copy link
Member

I added the min-width you suggested.

@cacqw7 that looks much better. I'd love to add a min-width for the patient name, too. But that's probably more of a global concern that we can address in another PR.

@clettenberg clettenberg force-pushed the add-requested-by-to-imaging branch from 4817ced to aa57ab3 Compare May 13, 2016 01:07
@clettenberg
Copy link
Contributor Author

@jkleinsc I removed those classes 👍

@jkleinsc
Copy link
Member

Looks good @cacqw7. I am going to merge it in.

@jkleinsc jkleinsc merged commit 722b0c1 into HospitalRun:master May 13, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants