Skip to content
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

feat: enhance context menu labels for branch lane actions #5856

Closed
wants to merge 1 commit into from

Conversation

PavelLaptev
Copy link
Contributor

@PavelLaptev PavelLaptev commented Dec 19, 2024

Reasoning:

Make the "Unapply" and "Unapply and drop changes" options clearer in different lane states.

Related issues: #5683


Here is how it looks:

image


Let me know if you have any improvements.

Copy link

vercel bot commented Dec 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
gitbutler-components ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 1, 2025 2:17am
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
gitbutler-web ⬜️ Skipped (Inspect) Jan 1, 2025 2:17am

@mtsgrd
Copy link
Contributor

mtsgrd commented Dec 21, 2024

@krlvi would now be a good time to start changing how this works? Even with these improvements by Pavel I find both the language and the current outcomes very confusing. Iirc the consensus is to make creating a local branch an explicit "export" action, which should resolve the ambiguity of what it means to unapply.

@Caleb-T-Owens
Copy link
Contributor

Looking back at the original issue, I'm a little disappointed in myself that I didn't spot this, and am sorry for causing extra work.

We've now got the button text changing based on whether or not the branch is pushed or not, which I'm trying to decide whether it makes sense.

Whether a branch is pushed or not has no effect on how the two options work.

The behaviours are as follows:

If no local branch exists for a lane:

  • Unapply & Save changes -> Writes out a local branch that contains those changes and the branch is gone from the board.
  • Unapply & Drop changes -> Changes are lost and the branch is gone from the board.

If a local branch exists for a lane:

  • Unapply & Save changes -> Updates the existing local branch to contain those changes and the branch is gone from the board.
  • Unapply & Drop changes -> Changes are lost and the branch is gone from the board. The existing local branch is left alone.

Comment on lines 56 to 57
const isVirginLane = $derived(
branch.name.toLowerCase().includes('lane') && commits.length === 0 && branch.files?.length === 0
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't make sense as a pushed/unpushed check, how do we tell if all of the branches in a given lane are integrated. Presumably we could use that mechanism.

@PavelLaptev PavelLaptev marked this pull request as draft December 21, 2024 22:34
@PavelLaptev
Copy link
Contributor Author

I see. I've converted this to a draft.
I don't have a strong opinion here. I just want to address the issue, because I also think that "unapply and drop changes" shouldn't exists or should be disabled if there are no changes.

btw Instead of disabling certain labels, I hid them. But maybe we should keep showing them in a disabled state instead.

@mtsgrd @Caleb-T-Owens, do you have any concrete suggestions regarding the labels?

@Caleb-T-Owens
Copy link
Contributor

As we did identify in the issue, if a branch has no commits or changes, then hiding/disabling Unapply & Save changes makes sense because there is nothing to save.

As for the other states, I'm not sure changing between Remove or Unapply conditionally makes sense. To me adding extra inconsistency

@PavelLaptev
Copy link
Contributor Author

As for the other states, I'm not sure changing between Remove or Unapply conditionally makes sense. To me adding extra inconsistency

Got it. Let me update this

@Caleb-T-Owens
Copy link
Contributor

I guess when it's an empty branch and is only showing Unapply & Drop changes then Remove might make more sense?

I do wonder if it would be preferable to keep it with the longer label, and have two tooltips that explain what the two actions do:

Write lane to git branch and remove lane from board & Remove lane from board without saving - or something like that.

@Caleb-T-Owens
Copy link
Contributor

Got it. Let me update this

Thank you for your patience!!!! ❤️

@PavelLaptev
Copy link
Contributor Author

@Caleb-T-Owens No problem! 😊

Could we also update the docs? I'm relying more on the docs with the new software rather than tooltips. It looks like the section on "applying and unapplying branches" is outdated.

Basically:

image

I'm also not sure if we're still using the term "Virtual branches."

image

@PavelLaptev PavelLaptev force-pushed the unapply-and-save-labels branch from 8d36bb4 to 9aaace5 Compare December 22, 2024 01:48
@vercel vercel bot temporarily deployed to Preview – gitbutler-web December 22, 2024 01:48 Inactive
@PavelLaptev
Copy link
Contributor Author

@Caleb-T-Owens updated variant
image

@Caleb-T-Owens
Copy link
Contributor

Why does it still have two extra states that depend on whether the lane has been pushed or not?

@PavelLaptev
Copy link
Contributor Author

Why does it still have two extra states that depend on whether the lane has been pushed or not?

I thought there were three conditions:

  1. Not pushed, no changes — Disable "Unapply" and rename "Unapply & drop changes" to "Remove lane"
  2. Pushed, no changes — Disable "Unapply & drop changes"
  3. All other cases — No disabled buttons or copy changes.

@Caleb-T-Owens
Copy link
Contributor

@PavelLaptev I laid out the behaviour of the two buttons in this message: #5856 (comment)

The behaviour doesn't change if you've pushed or not.

Does the behaviour of the buttons make sense?

@PavelLaptev
Copy link
Contributor Author

@Caleb-T-Owens I’m not changing the button behavior. According to this issue, if there are no changes and no pushed/local branches, rename "Unapply and drop changes" to "Remove" and disable "Unapply" because it doesn’t make sense.

@PavelLaptev PavelLaptev closed this Jan 7, 2025
@PavelLaptev
Copy link
Contributor Author

I closed this PR because we will address this more effectively in V3

@PavelLaptev PavelLaptev deleted the unapply-and-save-labels branch March 25, 2025 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants