Svelte: rev picker design updates and addition to commits page#63506
Conversation
| on:click={() => handleCopy()} | ||
| class="{buttonClass} copy-button hoverable-button" | ||
| > | ||
| <Icon icon={ILucideCopy} aria-hidden="true" --icon-size="1em" /> |
There was a problem hiding this comment.
FWIW since you added .hoverable-button to both of these buttons, you can also move the --icon-size="1em" declaration to the class below (single place) instead of passing it directly to the component.
| > *:not(:first-child) { | ||
| > .btn:not(:first-child), | ||
| > *:not(:first-child) .btn { |
There was a problem hiding this comment.
Previously, for this to trigger, it required that there was no element between the button group and the button. However, tooltips add a display: contents wrapper to target. This same pattern is used elsewhere in this file.
There was a problem hiding this comment.
Moved these up to a higher-level data loader so it can be used in the commits page.
| border-bottom: 1px solid var(--border-color-2); | ||
| margin: -0.75rem -0.75rem 0 -0.75rem; |
There was a problem hiding this comment.
Got rid of the negative margins in favor of just adding padding in the appropriate places.
| display: grid; | ||
| grid-template-rows: auto; | ||
| grid-template-columns: [title] auto [author] minmax(0, 10rem) [timestamp] minmax(0, 6rem); | ||
| grid-template-columns: [title] auto [author] minmax(0, 10rem) [timestamp] minmax(0, 8rem); |
There was a problem hiding this comment.
Increased the max size of the timestamp column so we aren't always truncating it
|
|
||
| .timestamp { | ||
| padding-right: 0.75rem; | ||
| justify-content: flex-end; |
There was a problem hiding this comment.
Right-aligning the timestamp looks a bit nicer IMO, since then ago (which is the same on every line) lines up
| display: grid; | ||
| grid-template-columns: min-content 1fr; | ||
| grid-template-areas: | ||
| 'collapse-button rev-picker' | ||
| 'search-files search-files'; | ||
| gap: 0.375rem; | ||
| padding: 0.5rem; |
There was a problem hiding this comment.
CSS changes here are mostly around using grid for the header rather than nested flexboxes.
| <Tooltip tooltip="{isCollapsed ? 'Open' : 'Close'} sidebar"> | ||
| <button class="{sidebarButtonClass} collapse-button" on:click={toggleFileSidePanel}> | ||
| <Icon icon={isCollapsed ? ILucidePanelLeftOpen : ILucidePanelLeftClose} inline aria-hidden /> | ||
| </button> | ||
| </Tooltip> |
There was a problem hiding this comment.
This all got dramatically simpler by not using the Button component and just using getButtonClassName
| // Prevent double borders when buttons are next to each other | ||
| margin-left: calc(-1 * var(--btn-border-width)); |
There was a problem hiding this comment.
moved this in here because the rule it was nested in before did not apply to buttons that were not direct children of the button group
|
@fkling sorry, I left a bunch of breadcrumb comments and forgot to actually submit them 😄 |
|
@camdencheek Great work! Quick design comment: I'd rather stick to using a secondary (or whatever button the original one is) in this case. The outline button adds more visual noise and I'd rather it stand out a bit like before. |
This cleans up the revision picker design, adds a copy button, and adds it to the file page.
Apologies for the big diff. I recommend reviewing with whitespace changes disabled.
Fixes SRCH-599
Fixes SRCH-460
Test plan
https://share.cleanshot.com/WlLhPqQd