Less flashy `closing-remarks` #6497

fregante · 2023-04-12T09:39:26Z

Fixes closing-remarks has wrong style #6482
Addresses Push less towards creating releases after merged PRs #5851 (comment)
May replace Change closing-remarks style #6489

Test URLs

PR: Fix and extend safer-destructive-actions on Discussions #5600
Locked PR: Add quote-props rule eslint/eslint#17
Archived repo: Issue #128 Replace ಠ with IIV fregante/iphone-inline-video#130
RGH tagged PR: Merged PR sandbox#1

Released

Unreleased

fregante · 2023-04-12T09:40:10Z

source/features/closing-remarks.tsx

 async function init(): Promise<void> {
 	const mergeCommit = select(`.TimelineItem.js-details-container.Details a[href^="/${getRepo()!.nameWithOwner}/commit/" i] > code`)!.textContent!;
 	const tagName = await getFirstTag(mergeCommit);

 	if (tagName) {
 		addExistingTagLink(tagName);
 	} else {
-		void addReleaseBanner('The merge commit doesn’t appear in any tags');
+		void addReleaseBanner('This PR’s merge commit doesn’t appear in any tags');


The copy was a little too vague

fregante · 2023-04-12T09:42:03Z

source/features/closing-remarks.tsx

-					text: <>This pull request first appeared in <span className="text-mono text-small">{tagName}</span></>,
-					classes: ['flash-success'],
-					action: tagUrl,
-					buttonLabel: <><TagIcon/> See release</>,


This widget only shows the state of the PR, it doesn't require any actions, so I think it'd make sense to not use the button.

This now also helps differentiate the two states:

released

unreleased

… because they used to be clearly blue and clearly green, but now their color varies very little.

fregante · 2023-04-12T09:43:03Z

source/features/closing-remarks.tsx

-					buttonLabel: <><TagIcon/> Draft a new release</>,
-				} : {text})}
+					buttonLabel: 'Draft a new release',
+				} : bannerContent)}


RequireAllOrNone types make optional properties annoying to use. Feel free to try locally

fregante · 2023-04-12T09:43:23Z

source/features/closing-remarks.tsx

 					action: url,
-					buttonLabel: <><TagIcon/> Draft a new release</>,
-				} : {text})}
+					buttonLabel: 'Draft a new release',


Icon gone, fixes closing-remarks has wrong style #6482

fregante · 2023-04-12T09:54:21Z

source/features/closing-remarks.tsx

-					classes: ['flash-success'],
-					action: tagUrl,
-					buttonLabel: <><TagIcon/> See release</>,
+					icon: <TagIcon className="m-0"/>,


The tag also aligns it well with rgh-netiquette in refined-github/sandbox#1

source/features/closing-remarks.tsx

Co-authored-by: Kid <44045911+kidonng@users.noreply.github.com>

kidonng · 2023-04-12T10:07:42Z

This is a good chance to refactor createBanner a bit:

refined-github/source/github-helpers/banner.tsx

Lines 12 to 13 in bac8aec

    
           // Classes copied from "had recent pushes" banner from repo home 
        
           const classes = 'flex-shrink-0 btn btn-sm ml-sm-3 mt-2 mt-sm-n2 mb-sm-n2 mr-sm-n1 flex-self-center';

- // Classes copied from "had recent pushes" banner from repo home
+ // https://primer.style/css/components/alerts#with-action-button
- const classes = 'flex-shrink-0 btn btn-sm ml-sm-3 mt-2 mt-sm-n2 mb-sm-n2 mr-sm-n1 flex-self-center'; 
+ const classes = 'btn btn-sm flash-action';

refined-github/source/github-helpers/banner.tsx

Lines 35 to 44 in bac8aec

    
           <div className={['flash', ...props.classes ?? ''].join(' ')}> 
        
           	<div className="d-sm-flex flex-items-center gap-2"> 
        
           		<div className="d-flex flex-auto flex-self-center flex-items-center gap-2"> 
        
           			{props.icon} 
        
           			{/* TODO: Drop `any` after https://github.com/frenic/csstype/issues/177 */} 
        
           			<span style={{textWrap: 'balance'} as any}>{props.text}</span> 
        
           		</div> 
        
           		{button} 
        
           	</div> 
        
           </div>

<div className={['flash', ...props.classes ?? ''].join(' ')}>
-	<div className="d-sm-flex flex-items-center gap-2">
-		<div className="d-flex flex-auto flex-self-center flex-items-center gap-2">
			{props.icon}
			{/* TODO: Drop `any` after https://github.com/frenic/csstype/issues/177 */}
			<span style={{textWrap: 'balance'} as any}>{props.text}</span>
-		</div>
		{button}
-	</div>
</div>

This was intended to be part of #6489

fregante · 2023-04-12T10:13:53Z

As the inline comment suggests, we're using GitHub’s own classes. I'm pretty sure that they lead to a better result than just that. See the current mobile version:

versus what I get (if I didn't make mistakes) by applying your change:

fregante · 2023-04-12T10:15:34Z

Basically that change would revert #5772

You can include this in your lint PR after this one is merged, if you can achieve the same result with semantic classes

fregante · 2023-04-12T10:20:06Z

Indeed 🙂

kidonng · 2023-04-12T10:46:20Z

That's float for you! (To be honest I encountered this in my attempt too)

It doesn't make sense to revert that PR. I will just create a new one to get rid of flash completely.

fregante · 2023-04-12T10:57:11Z

I will just create a new one to get rid of flash completely.

Reasoning?

kidonng · 2023-04-12T11:00:29Z

Reasoning?

Because we are overriding most things from that class?

fregante · 2023-04-12T11:32:35Z

Reasoning?

Because we are overriding most things from that class?

We're not? Borders, rounding, padding, colors, icon colors still come from flash. None of those classes (flex-shrink-0 btn btn-sm ml-sm-3 mt-2 mt-sm-n2 mb-sm-n2 mr-sm-n1 flex-self-center) actually override anything, we're only overriding the background right?

I suppose that if you can replicate what flash with Box and color-blue it might still be worth it, but if you're going to add 4 classes to replace 1… I don't think it's worth it. Also if we use utility classes instead of flash we lose any future update to flash

fregante · 2023-04-17T05:25:51Z

It looks like GitHub uses the same format and location to display status in open PRs:

The HTML

<div
  class="border color-border-default rounded-2 branch-action-item js-details-container js-transitionable"
>
 
  <div
    class="merge-status-list-wrapper"
  >
    <div
      class="float-left branch-action-item-icon completeness-indicator circle color-fg-muted border"
    >
      <RocketIcon/>
    </div>

    <h3 class="h4 status-heading">This branch has not been deployed</h3>
    <span class="status-meta"> No deployments </span>
  </div>
</div>

It looks decent on mobile but:

the icon isn't centered vertically (🤷‍♂️)
the button-related media-query classes are still missing

So it's still probably not worth spending a lot of time on it (or any) given the 50 open bugs 😅

fregante added 3 commits April 12, 2023 17:10

Less flashy closing-remarks

074646f

Add icon

3e1dcc1

Small refactor

14454a1

fregante added the change request label Apr 12, 2023

kidonng mentioned this pull request Apr 12, 2023

Change closing-remarks style #6489

Closed

fregante commented Apr 12, 2023

View reviewed changes

fregante requested a review from kidonng April 12, 2023 09:43

fregante commented Apr 12, 2023

View reviewed changes

Add extra demo link

f0d50cc

kidonng reviewed Apr 12, 2023

View reviewed changes

source/features/closing-remarks.tsx Outdated Show resolved Hide resolved

Thicker tag

9dcb654

Co-authored-by: Kid <44045911+kidonng@users.noreply.github.com>

kidonng approved these changes Apr 12, 2023

View reviewed changes

fregante merged commit f501e3c into main Apr 12, 2023
9 checks passed

fregante deleted the incospicuous-closing-remargs branch April 12, 2023 20:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Less flashy `closing-remarks` #6497

Less flashy `closing-remarks` #6497

fregante commented Apr 12, 2023 •

edited

fregante Apr 12, 2023

fregante Apr 12, 2023

fregante Apr 12, 2023

fregante Apr 12, 2023

fregante Apr 12, 2023

kidonng commented Apr 12, 2023 •

edited

fregante commented Apr 12, 2023

fregante commented Apr 12, 2023

fregante commented Apr 12, 2023

kidonng commented Apr 12, 2023 •

edited

fregante commented Apr 12, 2023

kidonng commented Apr 12, 2023

fregante commented Apr 12, 2023 •

edited

fregante commented Apr 17, 2023 •

edited

Less flashy closing-remarks #6497

Less flashy closing-remarks #6497

Conversation

fregante commented Apr 12, 2023 • edited

Test URLs

Released

Unreleased

fregante Apr 12, 2023

Choose a reason for hiding this comment

fregante Apr 12, 2023

Choose a reason for hiding this comment

fregante Apr 12, 2023

Choose a reason for hiding this comment

fregante Apr 12, 2023

Choose a reason for hiding this comment

fregante Apr 12, 2023

Choose a reason for hiding this comment

kidonng commented Apr 12, 2023 • edited

fregante commented Apr 12, 2023

fregante commented Apr 12, 2023

fregante commented Apr 12, 2023

kidonng commented Apr 12, 2023 • edited

fregante commented Apr 12, 2023

kidonng commented Apr 12, 2023

fregante commented Apr 12, 2023 • edited

fregante commented Apr 17, 2023 • edited

Less flashy `closing-remarks` #6497

Less flashy `closing-remarks` #6497

fregante commented Apr 12, 2023 •

edited

kidonng commented Apr 12, 2023 •

edited

kidonng commented Apr 12, 2023 •

edited

fregante commented Apr 12, 2023 •

edited

fregante commented Apr 17, 2023 •

edited