Closed PaulMorris closed 6 years ago
@PaulMorris, this is looking good to me. Thanks for doing it so quickly. Looks like you've got some tests to adjust, but that is perhaps expected -- I glanced quickly over the test failures and they seem to be just Selenium scripts that need to be updated now that certain pages have changed their appearance, not anything substantive.
My only question is about some of the changes in psm-app/cms-web/WebContent/css/style.css
that affect widely-used CSS, for example the change to .generalTable tr td
. Have you checked around other places in the PSM, both provider-view and admin-view, to make sure such CSS changes don't have any negative effects in places other than the tables in the screenshots? (The CSS changes look okay to me in principle, it's just that whenever I see changes made to core CSS I get a little nervous and compulsively ask questions.)
Thanks for the review @kfogel . Looks like the test failures are color contrast accessibility issues on two pages. (Disabled action column links in rows with the light grey background.) That's an easy fix.
Good question about the CSS. On the one hand, the changes that might affect other pages, beyond the tables that are part of this PR, are really small changes. (Just the additions to .generalTable tr td
and change to .dashboardPanel .tableData
.)
On the other hand there are 36 JSPs with a generalTable
class, and it would take some digging to hunt them all down to look at them. To play it safe I could introduce a new class, say linedTable
, with the changes and use that on the tables affected by this PR.
But that makes me sad as it would be better to have more consistent styling across all the tables, and these changes are so minor and probably a readability improvement. I'll think it over as I go and fix the contrast issues.
Going with safe path of introducing a new class linedTable
, given current constraints. It can be merged into generalTable
at some point if there's enough bandwidth to check all the appearance of all the generalTable
s.
Sounds good, @PaulMorris. I appreciate the caution. Let's try to remember that the option of merging the two, so that all tables are lined, is there, as we're looking at screens to choose screenshots.
@PaulMorris Hmmm, in fact, could you file an issue about the notion that linedTable should just become the default behavior for generalTable? That way we won't drop this on the floor.
@kfogel Sounds good. I'll file that issue. (I mentioned it in the commit message as well.)
And done, it's issue #1054.
Improve the spacing and styling of the links in Action columns in tables. Make table rows striped to make tables easier to read and make it easier to visually group Action column links. While we're at it, use a "View" link in the Action column rather than linking the text from another column, for system admin tables, and for Agreements and Addendums table.
Tested by looking at:
Before:
After:
Before:
After:
Before:
After:
Resolves #692: Action column may wrap to two lines