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

Improve CSV rendering #29638

Merged
merged 1 commit into from
Mar 10, 2024
Merged

Improve CSV rendering #29638

merged 1 commit into from
Mar 10, 2024

Conversation

silverwind
Copy link
Member

Before:

Screenshot 2024-03-06 at 21 42 17

After:
Screenshot 2024-03-06 at 21 41 58

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 6, 2024
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 6, 2024
@silverwind silverwind added the type/enhancement An improvement of existing functionality label Mar 6, 2024
@delvh
Copy link
Member

delvh commented Mar 6, 2024

How is removing the even-odd distinction an improvement?

@delvh
Copy link
Member

delvh commented Mar 6, 2024

But yeah, the single border looks good.

@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 Mar 6, 2024
@silverwind
Copy link
Member Author

silverwind commented Mar 6, 2024 •
edited
Loading

How is removing the even-odd distinction an improvement?

Just following Github style where they even-odd only in markdown tables, not csv. TBH, I'm not a fan of it anywhere.

@delvh
Copy link
Member

delvh commented Mar 6, 2024

That's one of the cases where I think it makes more sense to deviate from GitHub then.
After all, there is a reason for the different highlighting:
It helps the user keep track which row they are in.

@silverwind
Copy link
Member Author

silverwind commented Mar 6, 2024 •
edited
Loading

I really don't like it visually there because the even color is so similar to stuff around the table. Excel also does not do it 😆.

KN4CK3R
KN4CK3R reviewed Mar 10, 2024
View reviewed changes
Copy link
Member

@KN4CK3R KN4CK3R left a comment

Choose a reason for hiding this comment

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

OK for me because the csv preview is not intended to display files with 10/20/... columns. For a moderate number of colums even/odd should not be needed.

@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 Mar 10, 2024
@denyskon
Copy link
Member

@delvh Do you block?

delvh
delvh approved these changes Mar 10, 2024
View reviewed changes
Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

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

No.
But I don't like it either.

@6543 6543 merged commit 851bd18 into go-gitea:main Mar 10, 2024
26 checks passed
@GiteaBot GiteaBot added this to the 1.23.0 milestone Mar 10, 2024
@6543 6543 modified the milestones: 1.23.0, 1.22.0 Mar 10, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 11, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
139f55a 
* giteaofficial/main:
  Use repo object format name instead of detecting from git repository (go-gitea#29702)
  Improve CSV rendering (go-gitea#29638)
  Remove jQuery AJAX from the comment edit history (go-gitea#29703)
  fix: rendering internal file links in org (go-gitea#29669)
  Fix broken webhooks (go-gitea#29690)
  Suppress error from monaco-editor (go-gitea#29684)
@silverwind silverwind deleted the csv branch March 11, 2024 09:09
@silverwind silverwind added the backport/v1.21 This PR should be backported to Gitea 1.21 label Mar 12, 2024
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Mar 12, 2024
@silverwind @GiteaBot
Improve CSV rendering (go-gitea#29638)
f23eaf5 
Before:

<img width="1332" alt="Screenshot 2024-03-06 at 21 42 17"
src="https://github.com/go-gitea/gitea/assets/115237/0ea07eee-31f8-4783-bd56-37bd8396f00d">

After:
<img width="1336" alt="Screenshot 2024-03-06 at 21 41 58"
src="https://github.com/go-gitea/gitea/assets/115237/eb7f9cc9-587f-4e3b-92bd-cc67ca639963">
@GiteaBot GiteaBot mentioned this pull request Mar 12, 2024
Merged
@GiteaBot GiteaBot added the backport/done All backports for this PR have been created label Mar 12, 2024
silverwind added a commit that referenced this pull request Mar 12, 2024
@GiteaBot @silverwind
Improve CSV rendering (#29638) (#29744)
12b429c 
Backport #29638 by @silverwind

Before:

<img width="1332" alt="Screenshot 2024-03-06 at 21 42 17"
src="https://github.com/go-gitea/gitea/assets/115237/0ea07eee-31f8-4783-bd56-37bd8396f00d">

After:
<img width="1336" alt="Screenshot 2024-03-06 at 21 41 58"
src="https://github.com/go-gitea/gitea/assets/115237/eb7f9cc9-587f-4e3b-92bd-cc67ca639963">

Co-authored-by: silverwind <me@silverwind.io>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@KN4CK3R KN4CK3R KN4CK3R approved these changes

@yardenshoham yardenshoham yardenshoham approved these changes

@6543 6543 6543 approved these changes

@denyskon denyskon denyskon approved these changes

@delvh delvh delvh approved these changes

Assignees
No one assigned
Labels
backport/done All backports for this PR have been created backport/v1.21 This PR should be backported to Gitea 1.21 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.22.0
Development

Successfully merging this pull request may close these issues.
7 participants
@silverwind @delvh @denyskon @KN4CK3R @yardenshoham @6543 @GiteaBot