Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: improve result card reflow breakpoints #3372

Merged
merged 14 commits into from
Sep 18, 2020
Merged

fix: improve result card reflow breakpoints #3372

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
27f88c7
Convert cards to use display:flex instead of screen-width breakpoints
dbjorge Sep 16, 2020
d7937fd
Merge branch 'master' into cards-reflow-with-flex
dbjorge Sep 17, 2020
5d62755
Merge branch 'master' into cards-reflow-with-flex
dbjorge Sep 17, 2020
748344c
Clean up changes
dbjorge Sep 17, 2020
ad92555
Remove the global override label style
dbjorge Sep 17, 2020
27671bd
Update snapshots to account for removing override className
dbjorge Sep 17, 2020
6f38a2c
Reduce label width to make web nicer
dbjorge Sep 17, 2020
606781d
Expand label width to account for How to check in Needs review
dbjorge Sep 17, 2020
44057e7
Reflow a little faster to single-column
dbjorge Sep 17, 2020
51e487a
Expand row content flex basis per discussion with Fer
dbjorge Sep 17, 2020
acfe6a6
snippet css: fix typo and move to closer file
dbjorge Sep 18, 2020
6908083
update snapshots
dbjorge Sep 18, 2020
b96f21c
remove unnecessary flex-direction
dbjorge Sep 18, 2020
e0bd604
improve naming on card row styles
dbjorge Sep 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/common/components/cards/simple-card-row.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,7 @@ export const SimpleCardRow = NamedFC<SimpleCardRowProps>(

return (
<tr className={styles.row} key={rowKey}>
<th className={css(styles.label, 'report-instance-table-label-overrides')}>
{givenLabel}
</th>
<th className={styles.label}>{givenLabel}</th>
<td className={contentStyling}>{content}</td>
</tr>
);
Expand Down
4 changes: 0 additions & 4 deletions src/electron/views/automated-checks/automated-checks-view.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,4 @@
min-width: 220px;
}
}

:global(.report-instance-table-label-overrides) {
width: 127px;
}
}
112 changes: 49 additions & 63 deletions src/reports/components/instance-details.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,80 +51,66 @@
@include fill-available-width;

border-radius: inherit;

th {
padding: 12px 20px;

vertical-align: top;
}

.row {
top: calc(50% - 20px / 2);
border-bottom: 0.5px solid $neutral-10;
}

.row:last-child {
border-bottom: none;
}
}

.report-instance-table .label {
font-size: 14px;
line-height: 20px;
font-family: $semiBoldFontFamily;
color: $primary-text;
text-align: left;
width: 70px;
}

.report-instance-table .instance-list-row-content {
color: $secondary-text;
padding: 12px 20px;
font-size: 14px;
line-height: 20px;
align-items: flex-end;
display: flex;
}

.report-instance-table .instance-list-row-content.content-snipppet {
font-family: Consolas, Segoe UI, Courier New, monospace;
white-space: normal;
word-wrap: break-word;
word-break: break-word;
}

table.report-instance-table {
border-collapse: collapse;
overflow-x: auto;
word-break: break-word;
}

@media screen and (max-width: 480px) {
.report-instance-table tr {
display: block;
}

.report-instance-table td {
display: block;
overflow-wrap: break-word;
word-break: break-word;

.label {
text-align: left;
.row {
display: flex;
flex-direction: row;
dbjorge marked this conversation as resolved.
Show resolved Hide resolved
flex-wrap: wrap;

// The padding on the parent and margins on the children are coupled; they
// are intended to produce the desired spacing regardless of whether the children
// are presented side-by-side or wrapped
padding-top: 2px;
padding-bottom: 14px;
padding-left: 0px;
Copy link
Member

Choose a reason for hiding this comment

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

I tried removing this line and it had no effect on the styling. We might be able to get rid of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that it isn't currently necessary; I included it (and the 0 margins a few lines down) because I found it more readable to be explicit (there are some elements that browsers will use a 1px padding/margin for by default, and this saves a reader/reviewer from having to think about that/look up whether it applies)

padding-right: 20px;
> * {
margin-top: 12px;
margin-bottom: 0px;
margin-left: 20px;
margin-right: 0px;
Copy link
Member

Choose a reason for hiding this comment

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

I tried removing this line as well and it also had no effect on the styling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see above

padding: 0;
}

.instance-list-row-content {
text-align: right;
border-bottom: 0.5px solid $neutral-10;
Copy link
Member

Choose a reason for hiding this comment

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

nit: we can also write this like so:

        + .row {
            border-top: 0.5px solid $neutral-10;
        }

Adjacent elements can have styles applied to them like ($selector + $selector) to apply styles to all but the first element (hence why border-top over border-bottom). Yes, the nittiest of nits but I always like sharing this cool selector usage. Link to some text on it: https://alistapart.com/article/axiomatic-css-and-lobotomized-owls/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that this would work equally well, but I didn't use it because I think it's significantly harder for a reader to reason about, despite being shorter:

  • if someone don't know the advanced selectors off the top of their head, "last-child" is a lot easier to infer the meaning of than "+"
  • even if I do remember that "+" is an adjacent sibling selector, I have to remember whether it refers to the "left" or "right" sibling to understand if the css should be using -top or -bottom in a code review

&:last-child {
border-bottom: none;
}
}

.report-instance-table td::before {
float: left;
.label {
// This functions as the width of the label; it is sized to match the widest
// text we know the label to render in actual usage (currently "How to check"),
// plus 4px of fudge-factor to account for differences in fonts across browsers/OS
flex-basis: 90px;
flex-shrink: 1;
flex-grow: 0;

font-size: 14px;
line-height: 20px;
font-family: $semiBoldFontFamily;
color: $primary-text;
text-align: left;
}

.report-instance-table .label {
padding: 14px 20px 6px 20px;
.instance-list-row-content {
flex-basis: 200px;
flex-shrink: 1;
flex-grow: 1;

color: $secondary-text;
font-size: 14px;
line-height: 20px;
align-items: flex-end;
display: flex;
}

.report-instance-table .instance-list-row-content {
padding: 6px 20px 14px 20px;
.instance-list-row-content.content-snipppet {
dbjorge marked this conversation as resolved.
Show resolved Hide resolved
font-family: $codeFontFamily;
white-space: normal;
}
}
4 changes: 2 additions & 2 deletions src/tests/unit/tests/common/components/cards/__snapshots__/simple-card-row.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ exports[`SimpleCardRow renders 1`] = `
className="row"
>
<th
className="label report-instance-table-label-overrides"
className="label"
>
test label
</th>
Expand All @@ -22,7 +22,7 @@ exports[`SimpleCardRow renders with correct styling with extra class name 1`] =
className="row"
>
<th
className="label report-instance-table-label-overrides"
className="label"
>
test label
</th>
Expand Down
Loading