search

smart-on-fhir / growth-chart-app

Other
67 stars 98 forks source link

Fix styling on Entry Date row for the table view to display the entry date regardless if it is a same-day charting event #30

Open zplata opened 7 years ago

zplata commented 7 years ago

Currently, in the table view of the Growth Chart app, if there are 2+ different charting events on a patient within the same day, the first charting event will display the date on the "Entry Date" row, while the other charting events on the same day will display a dot instead of the full date (see screenshots below).

While the dot styling helps to show same-day events, it might be more clear to the user to just show the date instead of a dot. This way, someone looking at the time of the entry does not have to guess what exactly the dot represents on the table.

1_before 2_after
zplata commented 7 years ago

If the dot feature was also requested by physicians (similar to https://github.com/smart-on-fhir/growth-chart-app/pull/29), we could add a configuration flag for this feature too?

vlad-ignatov commented 7 years ago

Yes, it was requested by them but it was some kind of compromise. The issue was the following:

  1. By design, the first row should display the date and the second - the age. However, in this case they didn't like the date being repeated.
  2. If the time is rendered on the first row instead of date it was too confusing so they preferred to have it on the second row and to have this dot as a way of signifying that there is something special about that measurement...

In my opinion, there is only one reasonable and intuitive solution but it is more complicated. There might be 3 header rows instead of 2 - Date, Time and Age. Multiple records for the same day may have the time displayed on the second row and colspan=n for the date cell. The others can simply have rowspan=2 for the date cells (no time). This is difficult to implement however (considering that the table also supports row and column selections) and we don't plan to do it, unless we have some extra time to spare.

There is also a new version of the app that is currently being developed that might offer different solution.

zplata commented 7 years ago

@vlad-ignatov Ah, I see. If you think that this proposal still adds another viable option, we can add a configuration flag to this logic and flex both ways. Otherwise, we can close this PR out if flexing might add more complexity.

vlad-ignatov commented 7 years ago

Yes, I was trying to say that your PR does not exactly fix the UI/UX issues but if someone at Cerner prefers it that way, it won't hurt as long as it is optional.

zplata commented 7 years ago

@vlad-ignatov I have added the configuration flag that a user can choose to disable to change the display to what this PR is proposing.

zplata commented 7 years ago

@vlad-ignatov Please let me know if there's anything additional needing to be done to contribute this change back. Thank you!