search

treeverse / lakeFS

lakeFS - Data version control for your data lake | Git for data
https://docs.lakefs.io
Apache License 2.0
4.46k stars 355 forks source link

PRfD - Create PR page #8206

Closed itaigilo closed 1 month ago

itaigilo commented 1 month ago

Closes #8174.

Change Description

Adding the page for creating a Pull Request.

Screenshot 2024-09-22 at 16 08 04

github-actions[bot] commented 1 month ago

E2E Test Results - Quickstart

10 passed
github-actions[bot] commented 1 month ago

E2E Test Results - DynamoDB Local - Local Block Adapter

13 passed
itaigilo commented 1 month ago

Pull contents (title and description) deleted when changing branches

Since the url changes in this case, not sure how easy it is to implement this (I think it needs to be saved in the localStorage or something). I honestly don't think it's worth the effort for the MVP.

You can open a pull request even if there are no changes or it's the same branch (button should be greyed out)

True. I'll handle it.

Can we align the pull request description with the changes view (same width)

This might make sense, thanks. I'll play around with it to see how it looks.

Lets not show pull ID in the GUI (it's not aesthetic)

Yeah I was also wondering about it. If you also vote for removing it, I will.

We have a bad error when deleting a PR branch (we should print an informative message there and grey out the Merge button

Regarding the error message - do you think we should print a generic error instead? Something like "Pull Request creation failed"? O can you think of a nicer way of presenting server errors? And as for disabling the Merge button after error - I don't think we should. A user should be able to retry, plus they can just refresh the page and retry. So I think it should remain enabled.

N-o-Z commented 1 month ago

Pull contents (title and description) deleted when changing branches

Since the url changes in this case, not sure how easy it is to implement this (I think it needs to be saved in the localStorage or something). I honestly don't think it's worth the effort for the MVP.

You can open a pull request even if there are no changes or it's the same branch (button should be greyed out)

True. I'll handle it.

Can we align the pull request description with the changes view (same width)

This might make sense, thanks. I'll play around with it to see how it looks.

Lets not show pull ID in the GUI (it's not aesthetic)

Yeah I was also wondering about it. If you also vote for removing it, I will.

We have a bad error when deleting a PR branch (we should print an informative message there and grey out the Merge button

Regarding the error message - do you think we should print a generic error instead? Something like "Pull Request creation failed"? O can you think of a nicer way of presenting server errors? And as for disabling the Merge button after error - I don't think we should. A user should be able to retry, plus they can just refresh the page and retry. So I think it should remain enabled.

Thanks,

Regarding the last comment. What I meant was, after opening the pull request if one of the branches are deleted we get an ugly error in the changes view. We should optimally just provide a descriptive message saying one of the branches was deleted and disable the Merge button on that case. The same goes if there are no changes between the branches - allowing the user to try and merge when there are no changes is just not good user experience. I'm willing to defer the latter if it's too complicated for now but let's open an issue on it if so

itaigilo commented 1 month ago

All handled, Except for the comments related to empty diffs, which will be handled in a separate PR. Merging.