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

Add left padding for chunk header of split diff view #13397

Merged
merged 5 commits into from
Sep 18, 2021

Conversation

bagasme
Copy link
Contributor

@bagasme bagasme commented Nov 2, 2020

This commit adds 10px padding-left on chunk header element
(which is <span>).

For example (taken from commit 06268dc):
chunk header with padding

Without padding, the chunk header looks less neat:
chunk header without padding

Signed-off-by: Bagas Sanjaya bagasdotme@gmail.com

This commit adds 10px padding-left on chunk header element
(which is `<span>`).

Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com>
@codecov-io
Copy link

Codecov Report

Merging #13397 into master will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13397      +/-   ##
==========================================
- Coverage   42.13%   42.13%   -0.01%     
==========================================
  Files         691      691              
  Lines       75968    75968              
==========================================
- Hits        32010    32009       -1     
- Misses      38710    38714       +4     
+ Partials     5248     5245       -3     
Impacted Files Coverage Δ
services/pull/temp_repo.go 26.59% <0.00%> (-3.20%) ⬇️
services/pull/patch.go 53.97% <0.00%> (-1.71%) ⬇️
modules/queue/workerpool.go 58.77% <0.00%> (-1.23%) ⬇️
services/pull/pull.go 41.27% <0.00%> (+0.49%) ⬆️
modules/git/repo.go 46.70% <0.00%> (+0.50%) ⬆️
services/pull/check.go 49.63% <0.00%> (+0.72%) ⬆️
modules/git/utils.go 77.04% <0.00%> (+3.27%) ⬆️
modules/indexer/stats/db.go 60.86% <0.00%> (+8.69%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 06268dc...9d59019. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 2, 2020
@silverwind
Copy link
Member

silverwind commented Nov 2, 2020

Could also use the helper class pl-3 for similar effect without any CSS needed:

.pl-3 { padding-left: .5rem !important; }

Thought I guess you really want px-3 to pad both sides for symmetry:

.px-3 { padding-left: .5rem !important; padding-right: .5rem !important; }

@bagasme
Copy link
Contributor Author

bagasme commented Nov 4, 2020

@silverwind I'd tried your suggestion at commit bagasme/gitea@adda0dc.
Unfortunately, the resulting HTML after building does not change (the px-3 class is stripped away), despite that I'd cleared browser cache and doing make clean-all before build.

@silverwind
Copy link
Member

silverwind commented Nov 4, 2020

Try make clean-all followed by a make watch. There's no cache involved for HTML so it should always be fresh.

@bagasme
Copy link
Contributor Author

bagasme commented Nov 6, 2020

@silverwind I still can't get the px-3 class to appear on <span> chunk header element, and thus the padding.

@silverwind
Copy link
Member

Will try later. Unless there is JS that overrides it, I see no reason why it shouldn't work.

@silverwind
Copy link
Member

silverwind commented Nov 6, 2020

Where is that header seen? I thought it was diff but it seems that one has enought whitespace on the left side I think:

image

@bagasme
Copy link
Contributor Author

bagasme commented Nov 8, 2020

@silverwind that was what I mean for this PR

@silverwind
Copy link
Member

Isn't there enought whitespace already (added by unremovable .lines-type-marker):

image

@mrsdizzie
Copy link
Member

Hm seems like the screenshot in the PR description is misleading and has intentionally cropped out the existing whitespace from .lines-type-marker to the left then? I agree with @silverwind this seems to look OK already unless there is a misunderstanding of why changing it would be better. Looks about the same as Github now except they use @@ to start the line

Screen Shot 2020-11-09 at 11 11 17 AM

@stale
Copy link

stale bot commented Jan 10, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the issue/stale label Jan 10, 2021
@lunny lunny added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label Jan 11, 2021
@stale stale bot removed the issue/stale label Jan 11, 2021
@lunny lunny added issue/stale issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail labels Jan 11, 2021
@stale stale bot removed the issue/stale label Jan 11, 2021
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Aug 27, 2021
@zeripath
Copy link
Contributor

@mrsdizzie I've been able to replicate what @bagasme was seeing and I've fixed up this PR.

I think this is now ready for merge.

@zeripath zeripath added this to the 1.16.0 milestone Aug 27, 2021
@zeripath zeripath added topic/ui Change the appearance of the Gitea UI and removed issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented labels Aug 27, 2021
@GiteaBot GiteaBot removed the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Aug 27, 2021
@GiteaBot GiteaBot added the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Aug 27, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #13397 (51f4792) into main (d04e581) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #13397   +/-   ##
=======================================
  Coverage   45.25%   45.26%           
=======================================
  Files         771      771           
  Lines       86767    86767           
=======================================
+ Hits        39264    39272    +8     
+ Misses      41148    41140    -8     
  Partials     6355     6355           
Impacted Files Coverage Δ
modules/queue/queue_disk_channel.go 60.94% <0.00%> (-1.78%) ⬇️
services/pull/pull.go 41.78% <0.00%> (-0.41%) ⬇️
modules/queue/workerpool.go 47.70% <0.00%> (+0.38%) ⬆️
models/issue_comment.go 51.76% <0.00%> (+0.58%) ⬆️
services/repository/transfer.go 56.60% <0.00%> (+1.88%) ⬆️
modules/process/manager.go 75.30% <0.00%> (+2.46%) ⬆️
modules/git/utils.go 70.83% <0.00%> (+2.77%) ⬆️
modules/git/repo_base_nogogit.go 85.71% <0.00%> (+2.85%) ⬆️
modules/queue/queue_channel.go 95.00% <0.00%> (+3.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d04e581...51f4792. Read the comment docs.

@zeripath zeripath merged commit ea207f6 into go-gitea:main Sep 18, 2021
@go-gitea go-gitea locked and limited conversation to collaborators Oct 19, 2021
@bagasme bagasme deleted the chunk-header-padding branch May 5, 2022 04:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/ui Change the appearance of the Gitea UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants