Skip to content

Add view-last-pr-deployment feature#4089

Merged
fregante merged 20 commits intomainfrom
deploy
Mar 23, 2021
Merged

Add view-last-pr-deployment feature#4089
fregante merged 20 commits intomainfrom
deploy

Conversation

@yakov116
Copy link
Member

@yakov116 yakov116 commented Mar 12, 2021

Closes #2918

Test URLs

yakov116/pr-deployments#1
excalidraw/excalidraw#2993
primer/octicons#453 (a blast from the past ⭐)

Screenshot

image

@yakov116 yakov116 requested a review from fregante March 14, 2021 14:42
@yakov116
Copy link
Member Author

Do I get your approval?

@yakov116 yakov116 marked this pull request as draft March 15, 2021 13:44
@yakov116

This comment has been minimized.

@fregante
Copy link
Member

fregante commented Mar 16, 2021

Tooltip is redundant, deployment should be lowercase (I don’t think there's anything in Title Case on GH), the icon makes it feel like an action, I'd rather have the regular external icon on the right side as suggested

@yakov116
Copy link
Member Author

yakov116 commented Mar 16, 2021

Tooltip is redundant,

👍

deployment should be lowercase (I don’t think there's anything in Title Case on GH),

👍

the icon makes it feel like an action,

ok

I'd rather have the regular external icon on the right side* as suggested

GitHub style guide always has the icon on the left

@yakov116
Copy link
Member Author

image

@yakov116 yakov116 marked this pull request as ready for review March 16, 2021 23:48
@fregante
Copy link
Member

fregante commented Mar 17, 2021

The text is black or blue? I think black is ok

I'd rather have the regular external icon on the right side* as suggested

GitHub style guide always has the icon on the left

True

deployment should be lowercase (I don’t think there's anything in Title Case on GH),

👍

Copy link
Member

@fregante fregante left a comment

Choose a reason for hiding this comment

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

The comment above 👆 + every test link I opened showed 2 buttons in Chrome, at least the first time

Screen Shot 5

@yakov116
Copy link
Member Author

The comment above 👆 + every test link I opened showed 2 buttons in Chrome, at least the first time

I cannot understand why this is happening! I console.logged it and this observer runs 2 times yet on first-published-tag-for-merged-pr it does not 🤷

@fregante
Copy link
Member

fregante commented Mar 19, 2021

it runs 2 times yet on first-published-tag-for-merged-pr it does not 🤷

Probably it runs twice there too but then we have

https://github.com/sindresorhus/refined-github/blob/19330b2a4d1715c2d2496e6435a6b43263031c03/source/features/first-published-tag-for-merged-pr.tsx#L33

Maybe we should have an optimized on-conversation-header-update.ts? Written in a way that it can be used like this, maybe:

{
	additionalListeners: [
		onConversationHeaderUpdate
	],
	init
}

instead of

{
	init: () => onConversationHeaderUpdate(init)
}

@yakov116
Copy link
Member Author

image

Copy link
Member

@fregante fregante left a comment

Choose a reason for hiding this comment

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

Let's make the button text black

@yakov116
Copy link
Member Author

image

Copy link
Member

@fregante fregante left a comment

Choose a reason for hiding this comment

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

You can make this change either here or somewhere else. It looks good otherwise. I hope people don’t complain about wrapping in small viewports

@yakov116
Copy link
Member Author

I hope people don’t complain about wrapping in small viewports

We can hide it...

@fregante
Copy link
Member

The position was a little too important in small windows
Screen Shot 2

So I hid it for now

@dwelle
Copy link

dwelle commented Mar 23, 2021

Thanks @yakov116 & @fregante! ❤️

@fregante
Copy link
Member

fregante commented Apr 12, 2021

Screen Shot 5

Bug when the Edit button is visible. Can probably be fixed by a single space in the dom

@yakov116
Copy link
Member Author

Bug when the Edit button is visible. Can probably be fixed by a single space in the dom

Until the next header update.

I could not always get it to do that. Sometimes yes sometimes no. 🤷‍♂️

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

Development

Successfully merging this pull request may close these issues.

Deployment shortcut in pull request headers

3 participants