-
Notifications
You must be signed in to change notification settings - Fork 695
Fix CommitTitle to show context-appropriate messages for empty commits #10406
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.
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
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
This way we can show empty commits without the 'drag changes here' text. Co-authored-by: Byron <[email protected]>
13202f1
to
945ef1c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disabled, | ||
hasConflicts, | ||
active, | ||
editable = true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, let's remove this default value here. The type is already editable?: boolean;
so instances don't need the prop specified as false. This means we could remove the explicit editable={false}
rows in this pr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review! I think I have addressed the this as well as I could, given that… somehow editable = false
was needed so editable
wouldn't be truthy by default. (I don't understand what's going on here, I hope I am missing something)
In any case, I hope you will merge this PR after taking it to the finishing line 🙏.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure thing, I can I can take of the rest, just ignore my new comment! It shouldn't be truthy by default, but I can make sure. :)
disabled, | ||
hasConflicts, | ||
active, | ||
editable = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome, i see you deleted the other code. could we still make this just editable,
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is just editable, then it won't work anymore like it's defaulting to true
or something truthy. That was my problem, the thing I didn't understand 😁.
@mtsgrd Is this PR OK to merge? |
The
CommitTitle
component was showing misleading "Empty commit. Drag changes here" messages in read-only contexts where dragging is not actually possible, such as inBranchCommitList
whenstackId
is undefined.Problem
Previously, all empty commits displayed the same message regardless of context:
This was confusing in read-only contexts like:
BranchCommitList
whenstackId
is undefinedTargetCommitList
(always read-only)UnappliedCommitView
(unapplied commits)Solution
Added an optional
editable
prop toCommitTitle
that defaults totrue
for backward compatibility:Updated all usage locations to pass appropriate
editable
values:CommitView
passeseditable={!isReadOnly}
based onstackId
presenceBranchCommitList
,TargetCommitList
, etc. passeditable={false}
BranchCommitList
passeseditable={!!stackId}
since it's only editable when astackId
existsResult
The fix ensures users only see drag instructions when dragging is actually possible, improving the overall user experience and reducing confusion.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.