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

Use only first line of commit when creating referenced comment #11960

Merged
merged 4 commits into from
Jun 19, 2020

Conversation

CirnoT
Copy link
Contributor

@CirnoT CirnoT commented Jun 18, 2020

We should only include first line. Ideally we'd use some form of ellipsis but that would require some heavy refactoring, as right now we store anchor HTML in database.

Before:
chrome_2020-06-18_14-00-34

After:
chrome_2020-06-18_14-00-45

@CirnoT
Copy link
Contributor Author

CirnoT commented Jun 18, 2020

The alternative here is to write a migration that extracts actual content from anchor link in database and save it without it, then we render it as we do other commits with ellipsis

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 18, 2020
@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 Jun 18, 2020
@lafriks lafriks added the topic/ui Change the appearance of the Gitea UI label Jun 18, 2020
@lafriks lafriks modified the milestones: 1.x.x, 1.13.0 Jun 18, 2020
Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

would say this is a bugfix witch we can backport to v1.12.1

@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 Jun 18, 2020
@6543
Copy link
Member

6543 commented Jun 18, 2020

@CirnoT beside this you could refactor it for v1.13 in a new pull :)

@CirnoT
Copy link
Contributor Author

CirnoT commented Jun 18, 2020

@6543 If we do backport, we backport is instead of this PR, otherwise we'll lose additional data for new commits.

@CirnoT
Copy link
Contributor Author

CirnoT commented Jun 18, 2020

Alternatively, we could merge this for v1.12.1 backport and refactor to store only commit hash in database and fetch commit message on-the-fly for v1.13.

@silverwind
Copy link
Member

silverwind commented Jun 18, 2020

Can you also fix this on the frontpage news feed maybe?

image

@6543
Copy link
Member

6543 commented Jun 18, 2020

I would display the first line and make a a "> more" in the next line
witch works similar to the "Downloads" on the release page?

@silverwind
Copy link
Member

make a a "> more"

We do support a ... button on the repo summary page for the top commit. That said I think it's fine to not have such button everywhere where a commit can appear.

@mrsdizzie
Copy link
Member

Think it would look bad and be clunky to have any kind of expanding 'read more' button. Thats why it already links to the commit page : )

@6543
Copy link
Member

6543 commented Jun 18, 2020

@zeripath whats the point for SplitN since the rest will be cut away in all cases? - does it use less memory?

@6543
Copy link
Member

6543 commented Jun 18, 2020

Think it would look bad and be clunky to have any kind of expanding 'read more' button. Thats why it already links to the commit page : )

good point - so i think we dont need any refactor just display only the first line

@CirnoT
Copy link
Contributor Author

CirnoT commented Jun 18, 2020

Can you also fix this on the frontpage news feed maybe?

Definitely should, let me take a quick look

@CirnoT CirnoT marked this pull request as draft June 18, 2020 21:22
@zeripath
Copy link
Contributor

@6543 what's the point of reading the rest of the string splitting it in to every line when you don't care about the other lines?

SplitN reads until it has the requested number of splits and stops. Split reads the entire string.

@CirnoT CirnoT marked this pull request as ready for review June 18, 2020 21:36
@CirnoT
Copy link
Contributor Author

CirnoT commented Jun 18, 2020

Should be fixed on feeds now too

Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

Frontpage working, thanks!

@lunny lunny merged commit 92a05f8 into go-gitea:master Jun 19, 2020
@CirnoT CirnoT deleted the ref-singleline branch June 19, 2020 07:28
ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
…tea#11960)

* Use only first line of commit when creating referenced comment

* Update modules/repofiles/action.go

* Display first line only on feeds too

Co-authored-by: techknowlogick <[email protected]>
Co-authored-by: zeripath <[email protected]>
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
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.

9 participants