Less attention grabbing hint to draft a new release#5854
Conversation
|
It looks good, but I’m not sure the icon works there. Should we use .Btn-link on the A tag? |
|
I can agree, the icon is a little weird for an inline link. I will remove. Adding |
|
Not a super fan of how it looks/aligns to be honest 😅
I added some spacing next to the link in my last commit |
There was a problem hiding this comment.
I think we should use the same style/alignment as the other two events visible in my screenshot above, including the button (but no horizontal rule/line)
or this "message bubble" style:
HTML
<form class="ml-0 pl-0 ml-md-6 pl-md-3 my-3 branch-action branch-action-state-closed-dirty js-immediate-updates js-needs-timeline-marker-header" data-turbo="false" action="/xojs/xo/pull/620/cleanup" accept-charset="UTF-8" method="post"><input type="hidden" name="authenticity_token" value="">
<span class="branch-action-icon d-none d-md-flex flex-items-center flex-justify-center">
{svgicon}
</span>
<div class="branch-action-body timeline-comment--caret">
<div class="post-merge-message">
<div class="Details">
<h4 class="merge-branch-heading">
Pull request closed
</h4>
<p class="merge-branch-description">
<span>
If you wish, you can delete this fork of <strong>xojs/xo</strong> in the <a href="/fregante/xo/settings?confirm_delete=yes">settings</a>.
</span>
</p>
</div>
</div>
</div>
</form>|
Yeah, good points. The inconsistency is also not nice, it becomes more busy if we add yet another style. Since it is not an event the banner styling is more correct. If I understand you correctly, we can just keep the styling as is, but remove the background color? (Like in the 'Pull request closed' banner, but then with the button and right-floating as currently.) |
|
I think we can reuse the whole markup I pasted. It's essentially the same concept:
Generally these two won't appear at the same time because people who send PRs from temporary forks aren't the same who can create a release. The same bubble style could also be ok for every usage in the feature though. |
|
I'm not sure I understand. The screenshot you attached, looks the same as the current situation, doesn't it? |
I'm talking about the "Pull request closed" style. It's true that it's similarly-sized, but it's not colored and it's what GitHub uses for a very similar intent: You can do something now that the PR has been merged. |
|
Or like this, which I mentioned as an alternative:
|
|
Ah, I understand. Some thoughts I have about these:
I'm more inclined to keep the same situation as now, but with a transparent instead of a blue background. This:
Although, thinking about the last point. Deleting a fork is less important than drafting a release. So maybe the extra attention is worth it. I'm also fine closing this PR. I think it was good that the banner was removed if a repo doesn't have releases at all, with that this might be good as is. |
Super rare. People who can release a new versions don't generally use forks. If they do, that's their own issue 😃
I agree, that's why the bubble works, exactly like GitHub is doing.
That works for me, it's also the easiest probably, except that I'm not sure how to safely override the Banner styles. 🤔
That's ok, someone else can do it too. |
|
Feel free to send a new PR |
Fixes #5851.
On merged PRs which are not (yet) part of a release a banner is shown to hint to create a release. This PR changes it from a banner & button, to regular text & link.
I also changed the wording a little, but if this is not wished it can be reversed of course. I did it can be confused for other forms of 'releasing', like deploying. I didn't remove the word 'release', just used other language to make it more understood as a noun.
I'm unsure about the conventions of html elements & classes. I borrowed a class from GitHub to make the text color a little more muted. Is that okay or should a RG class be made & used?
Test URLs
It can be seen on any merged PR which is not part of a release. Note that due to #5850, the banner/text isn't shown when the repository doesn't use releases at all.
Current example: #5850.
Screenshots
From current situation:
To new situation:
