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

Revert increased width on pull pages #21470

Merged
merged 9 commits into from
Oct 19, 2022
Merged

Revert increased width on pull pages #21470

merged 9 commits into from
Oct 19, 2022

Conversation

Gusted
Copy link
Contributor

@Gusted Gusted commented Oct 15, 2022

Before

image
image

After

image
image

Revert increased width on pull pages
7dce8a5 
- Revert a behavior from go-gitea#21012, which liberally added `fluid padded` to
non-split style pull pages, this caused it to take up the whole
screen(such in split-style pull pages) on pull pages where the diff was
shown.
- Resolves go-gitea#21460
@Gusted Gusted added issue/regression Issue needs no code to be fixed, only a description on how to fix it yourself skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Oct 15, 2022
@Gusted Gusted added this to the 1.18.0 milestone Oct 15, 2022
wxiaoguang
wxiaoguang approved these changes Oct 16, 2022
View reviewed changes
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

LGTM.

My screen might be not wide enough at that moment so I didn't feel unusual 😁

Maybe there could be some comments about why the IsSplitStyle is needed.

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Oct 16, 2022
@lunny
Copy link
Member

lunny commented Oct 16, 2022

Since there are some diff UI in this page, I don't think making it narrow is a good idea.

@Gusted
Copy link
Contributor Author

Gusted commented Oct 16, 2022

Since there are some diff UI in this page, I don't think making it narrow is a good idea.

It was already narrow to begin with, but for some reason it was removed for the "unified" view(without any reason mentioned on the PR), which doesn't need that much space IMO.

I think if that's the design choice we want to go with, it should be adjusted as currently other elements are also being affected that shouldn't be affected(see Labels/Milestone buttons). But I don't see a lot of good reasons to increase the width of the diff one.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 18, 2022
@silverwind
Copy link
Member

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.

@zeripath zeripath merged commit 6b71246 into go-gitea:main Oct 19, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request Oct 21, 2022
@zjjhot
Merge remote-tracking branch 'upstream/main'
8c21404 
* upstream/main:
  [skip ci] Updated translations via Crowdin
  Check for valid user token in integration tests (go-gitea#21520)
  Ignore error when retrieving changed PR review files (go-gitea#21487)
  move invite by mail to services package (go-gitea#21513)
  Enable Monaco automaticLayout (go-gitea#21515)
  Update macOS install command (go-gitea#21507)
  [skip ci] Updated translations via Crowdin
  Suppress `ExternalLoginUserNotExist` error (go-gitea#21504)
  Revert increased width on pull pages (go-gitea#21470)
  Add team member invite by email (go-gitea#20307)
  Disable the 'Add File' button when not able to edit repo (go-gitea#21503)
  Remove vitest globals (go-gitea#21505)
  Fix branch dropdown shifting on page load (go-gitea#21428)
@wxiaoguang wxiaoguang mentioned this pull request Feb 9, 2023
Closed
@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

@silverwind silverwind silverwind approved these changes

@lafriks lafriks lafriks approved these changes

@wxiaoguang wxiaoguang wxiaoguang approved these changes

Assignees
No one assigned
Labels
issue/regression Issue needs no code to be fixed, only a description on how to fix it yourself lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features.
Projects
None yet
Milestone
1.18.0
Development

Successfully merging this pull request may close these issues.

Regression for width on pull request
7 participants
@Gusted @lunny @silverwind @lafriks @wxiaoguang @zeripath @GiteaBot