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

New commits list #24423

Closed
wants to merge 4 commits into from
Closed

New commits list #24423

wants to merge 4 commits into from

Conversation

silverwind
Copy link
Member

Very early WIP:

Screenshot 2023-04-29 at 13 42 06

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 29, 2023
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 29, 2023
@silverwind silverwind added type/enhancement An improvement of existing functionality topic/ui Change the appearance of the Gitea UI and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 29, 2023
@silverwind silverwind marked this pull request as draft April 29, 2023 11:44
@silverwind silverwind changed the title WIP: New commits list New commits list Apr 29, 2023
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 29, 2023
@silverwind
Copy link
Member Author

I will also re-work the commit badge, likely removing the "fancy" styling around it and just use icons to indicate signature. Part of the styling for it is unfortunately shared with last commit in file table, so extensive refactoring will be required to decouple those two ways of displaying a commit.

@lunny
Copy link
Member

lunny commented Apr 29, 2023

I think line dividers are still needed.

<div class="ui attached table segment commits-list">
{{$commitRepoLink := $.RepoLink}}{{if $.CommitRepoLink}}{{$commitRepoLink = $.CommitRepoLink}}{{end}}
{{range .Commits}}
<div class="commits-list-row gt-df gt-ac gt-px-4 gt-py-2">
Copy link
Contributor

Choose a reason for hiding this comment

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

These "gt-df gt-ac gt-px-4 gt-py-2" seems quite wordy.

Since it has a class commits-list-row, is it better to put the styles on the particular CSS class name?

Copy link
Member Author

Choose a reason for hiding this comment

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

As mentioned in the other issue, I think what can be done with helper classes should be done with them. I see CSS currently only for stuff that can not be done with helper classes.

Another benefit of helper classes is that they move when the element moves, so won't leave garbage CSS during refactoring.

I do like to retain descriptive class names, primarily as a visual aid for reading the code.

Choose a reason for hiding this comment

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

I think what can be done with helper classes should be done with them.

Totally agree. It helps when working on other people's code.

I do like to retain descriptive class names, primarily as a visual aid for reading the code.

Interesting idea. I'll give that a try,

silverwind added a commit that referenced this pull request May 3, 2023
Make this stylelint rule match on more properties.

The dead CSS relates to the navbar, which currently has classes:

```
ui top secondary stackable main menu following bar light
```

Which means `.following.bar .top.menu` can never match, so remove this
dead CSS as well as inactive `z-index` and `left` on it.

Commits table striping becomes more visible on dark theme, but I don't
think it's worth introducing a new color until
#24423 is ready, which would have
to remove it again:

<img width="668" alt="Screenshot 2023-05-01 at 18 41 49"
src="https://user-images.githubusercontent.com/115237/235489873-6b272899-1d78-443a-872c-ee7731c269f9.png">
<img width="680" alt="Screenshot 2023-05-01 at 18 41 41"
src="https://user-images.githubusercontent.com/115237/235489878-1b9468af-c74f-48a6-a469-9eba57cfcb4d.png">
@delvh
Copy link
Member

delvh commented Sep 3, 2023

Will it ever get ready for review?

@silverwind
Copy link
Member Author

Maybe. It is pretty high on my list of things to do.

@silverwind
Copy link
Member Author

I think I'll first split out a PR that moves or removes the overly prominent styling of commits hashes, e.g. green/yellow/red for signature level. I think we should definitely subtemplate these which in turn will make the commit list implementation much simpler.

@silverwind
Copy link
Member Author

Not going to continue here, maybe later in a different PR, but anyone can feel free to pick this up in the meanwhile.

@silverwind silverwind closed this Mar 23, 2024
@silverwind silverwind deleted the commit-list branch March 23, 2024 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. topic/ui Change the appearance of the Gitea UI type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants