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.

Sign up for GitHub

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

Jump to bottom

Add button to change width of diff view #22824

Closed
wants to merge 7 commits into from
Closed

Add button to change width of diff view #22824

wants to merge 7 commits into from

Conversation

gempir
Copy link
Contributor

@gempir gempir commented Feb 8, 2023

Fixes #22781

This adds a button that let's you configure the width of the diff viewer. I think this feature shouldn't be tied to the unified/split view button so now it's entirely separate.

I used the lock/unlock Icon because it was available. If anyone has a better Icon I'm welcome to change it. I'm unsure where you are sourcing your icons from though or who is designing them, they look quite nice.

image
image

@wolfogre
Copy link
Member

wolfogre commented Feb 9, 2023

It looks good! However, since you changed a database table, you need to add a migration to models/migrations/, here's an example: https://github.com/go-gitea/gitea/pull/20908/files#diff-8ba39375dd03644c2b046bbd1654aaaec4a84d75ef009ef10597b0288250db06

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 9, 2023
@lunny lunny added the topic/ui Change the appearance of the Gitea UI label Feb 9, 2023
@lunny lunny added this to the 1.20.0 milestone Feb 9, 2023
lunny
lunny reviewed Feb 9, 2023
View reviewed changes
routers/web/repo/middlewares.go Outdated
}

ctx.Data["IsWidthFull"] = width == "full"
if err := user_model.UpdateUserDiffViewWidth(ctx.Doer, width); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

This should be a temporary change, so we shouldn't change the user's preference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this should be permanent. Just like split/unified view. Personally I want it always to be full.

@lunny
Copy link
Member

lunny commented Feb 9, 2023

Since we have user_setting table, I think we should not create a new column in user table.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Feb 9, 2023 •
edited
Loading

This problem seems to be somewhat more complex than it looks. Some backgrounds:

  1. It has been changed to full width by Add filetree on left of diff view #21012
  2. Then the width is reverted by Revert increased width on pull pages #21470
    • Because the Pull Request page is also affected by 21012
    • But this PR reverted all.

In my opinion:

I think we can discuss going full width for certain pages (GitHub also does full width on unified diff), but it needs to be done more carefully without affecting things like this did with the issue/pr page.

@gempir
Copy link
Contributor Author

gempir commented Feb 9, 2023

This problem seems to be somewhat more complex than it looks. Some backgrounds:

  1. It has been changed to full width by Add filetree on left of diff view #21012

  2. Then the width is reverted by Revert increased width on pull pages #21470

    • Because the Pull Request page is also affected by 21012

    • But this PR reverted all.

In my opinion:

I think we can discuss going full width for certain pages (GitHub also does full width on unified diff), but it needs to be done more carefully without affecting things like this did with the issue/pr page.

Just having it full width without any setting would be my preference too, but I imagine it's quite a difference for the user so I thought I'll offer an option.

I don't really know how to continue on in this PR is there a consensus on just removing the compact view? Or should I enhance my setting approach so it's mergeable.

@wxiaoguang
Copy link
Contributor

Usually most liked solution wins.

And there is a Technical Oversight Committee could help to make final decisions if I understand correctly.

@gempir gempir mentioned this pull request Feb 9, 2023
Merged
@gempir
Copy link
Contributor Author

gempir commented Feb 9, 2023 •
edited
Loading

I changed it to use the user_settings table. I also opened another PR solves the problem a lot "simpler" by just making everything full width. #22844

I also went and edited other pages that use the diff view like the compare page (new PRs) and commit page

Either of them would solve my issue #22781

jolheiser pushed a commit that referenced this pull request Feb 16, 2023
@gempir
Always go full width in PR view (#22844)
d9c6cb7 
This is an alternative solution to #22824 
and would also close #22781

This makes the PR diff view always full width.
It makes sense to make use of that screen real estate. If you want a
more narrow view you can always resize your browser.
It also avoids cluttering the UI with another button + the database with
another column for the setting.

This is also how github and gitlab do it.
@gempir
Copy link
Contributor Author

gempir commented Feb 16, 2023

As #22844 has been merged. I will close this PR.

@gempir gempir closed this Feb 16, 2023
@lunny lunny removed this from the 1.20.0 milestone Feb 17, 2023
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lunny lunny lunny left review comments

Assignees
No one assigned
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. topic/ui Change the appearance of the Gitea UI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Full Width Files Changed View Unified
5 participants
@gempir @wolfogre @lunny @wxiaoguang @GiteaBot