Skip to content

ui: update styles for session details page#55499

Merged
craig[bot] merged 4 commits intocockroachdb:masterfrom
koorosh:ui-session-page-style-update
Oct 22, 2020
Merged

ui: update styles for session details page#55499
craig[bot] merged 4 commits intocockroachdb:masterfrom
koorosh:ui-session-page-style-update

Conversation

@koorosh
Copy link
Copy Markdown
Contributor

@koorosh koorosh commented Oct 13, 2020

resolves #54513
depends on cockroachdb/admin-ui-components#31
depends on cockroachdb/admin-ui-components#34
depends on cockroachdb/admin-ui-components#35

Current fix is quite complex and touches a lot of places because most of the issues with styles were common for all pages (Statements, Jobs, Node Overview).
Also many style duplicates required to get rid of duplicates and unify single version of proper components to use across entire app.

It's easier to review this PR per commit. Each commit describes in details what changes were made and their purpose.

Screen Shot 2020-10-13 at 3 18 18 PM

Screen Shot 2020-10-13 at 1 43 23 PM

@koorosh koorosh requested a review from a team October 13, 2020 11:53
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@Annebirzin
Copy link
Copy Markdown

One language thing I notice here in the screenshots: We use terminate statement and terminate session in the session list view and we use cancel query and cancel session on the detail page.

@awoods187 @jordanlewis can you confirm that we want to go with terminate statement and terminate session for both? cc @taroface

Copy link
Copy Markdown
Contributor

@nathanstilwell nathanstilwell left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r1, 9 of 9 files at r2, 9 of 9 files at r3, 4 of 4 files at r4.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @koorosh)


pkg/ui/src/views/sessions/sessionsPage.module.styl, line 11 at r3 (raw file):

// licenses/APL.txt.

@require '~src/views/statements/statementsPage.module.styl'

hmmm. I feel like we could move these styles into a more central place if they are going to be used by both Statements and Sessions.

@koorosh koorosh force-pushed the ui-session-page-style-update branch from 213a6c8 to 3fd8be8 Compare October 16, 2020 11:08
@koorosh
Copy link
Copy Markdown
Contributor Author

koorosh commented Oct 16, 2020

Reviewed 5 of 5 files at r1, 9 of 9 files at r2, 9 of 9 files at r3, 4 of 4 files at r4.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @koorosh)

pkg/ui/src/views/sessions/sessionsPage.module.styl, line 11 at r3 (raw file):

// licenses/APL.txt.

@require '~src/views/statements/statementsPage.module.styl'

hmmm. I feel like we could move these styles into a more central place if they are going to be used by both Statements and Sessions.

Took a closer look and figured out that statementsPage.module.styl isn't used anymore in Statements page, it was left as a redundant style file when we migrated from global styles to CSS modules. As a solution I moved it to SessionsPage directory where it is used exclusively.
Right now, I don't know what is the best way to define shared styles for these kinds of definitions, because current styles don't represent any particular component. Instead these styles just customize components visual representation.

Copy link
Copy Markdown
Contributor

@nathanstilwell nathanstilwell left a comment

Choose a reason for hiding this comment

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

Reviewed 21 of 21 files at r5, 9 of 9 files at r6, 9 of 9 files at r7, 6 of 6 files at r8.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

Before, Session page has filters menu which used default Antd component
and it deviated common styles.

Now, existing `Dropdown` component is extended to be used in Sessions
page with extra properties to get more flexibility for customization.
And this Dropdown is used instead.

Release note: none
Previously, "back links" from details page to main
pages had regression on styles and didn't satisfy
current designs.

Now, links by default black and have blue color on
hover only. Also styles for links inside SortedTable
component is fixed to inherit font size and font styles
from parent elements

Release note (admin-ui-change): Fix link colors for "Back" link on
Details pages (Node Overview, Jobs, Sessions, and statements
details pages.
Previously, `Pagination` component was used from Antd design system
and every page component added it's own duplicate of custom styles
on top of Antd Pagination. This introduced multiple places which
have to changed in order to unify styles across entire app.

Now, main `Pagination` component is added to `admin-ui-components`
package which is basically wrapper on top of Antd component with
custom styles. Also, all existing duplicates for styles are removed
and it avoids confusion to know which Pagination is correct one.

In addition, styles which were duplicated from StatementsPage into
SessionsPage were re-imported in it's own styles to keep single
places for changes.

Release note: none
Session details page had inconsistent styles where headers for sections
had wrong sizes, spacings between sections were larger than in designs,
and positions for action buttons (Cancel query and session buttons)
had to be placed in header section.

Now, styles are adjusted according to designs and fixed in a way to
avoid global changes. The only one global change is made to reset
`line-height` property for H1 element which were inherited from <body>
element and caused wrong text positioning.

Release note (admin ui change): adjusted styles for Session details page
@koorosh koorosh force-pushed the ui-session-page-style-update branch from 3fd8be8 to ee97a25 Compare October 21, 2020 09:31
@koorosh
Copy link
Copy Markdown
Contributor Author

koorosh commented Oct 22, 2020

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Oct 22, 2020

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ui: Sessions pages style updates

5 participants