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

Prettify Timeline #10972

Merged
merged 9 commits into from
Apr 10, 2020
Merged

Prettify Timeline #10972

merged 9 commits into from
Apr 10, 2020

Conversation

Sorien
Copy link
Contributor

@Sorien Sorien commented Apr 5, 2020

it should looks like
screencapture-localhost-8888-Sorien-my-pulls-1-2020-04-05-15_01_00

  • Fixes timeline icons, offset, background
  • Review comments to be github like
  • Margins and Padding all over the place and so on ...

fixes
#10864

replaces
a1012112796@5b7addb

Co-authored-by: a1012112796 1012112796@qq.com

@a1012112796
Copy link
Member

@Sorien Please fix lint.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 5, 2020
@Sorien Sorien force-pushed the timeline2 branch 2 times, most recently from b26f3c5 to 51e2511 Compare April 5, 2020 18:25
@mrsdizzie
Copy link
Member

Thank you for PR I will try and review later. If you aren't sure how to fix the failing tests you can add me as a collaborator and I can fix them

@Sorien
Copy link
Contributor Author

Sorien commented Apr 5, 2020

@mrsdizzie I added you as a collaborator, it would be great if you could fix these tests :)

@mrsdizzie
Copy link
Member

Thanks I'll take a look -- also its best to avoid force pushing to PRs (it will mess up reviews and makes it harder to see certain changes)

@Sorien
Copy link
Contributor Author

Sorien commented Apr 5, 2020

I was just fixing random things and rebasing multiple times, I'm mostly done, for now.
It should be ready for review.

@a1012112796
Copy link
Member

Hello, I have test your result now. It's great, Thanks. but It have small problem I think. the gap between the two lines is too small. Could you please make it a little bigger?That will be more better. Thanks

your view :
testm

github:
testd

@codecov-io
Copy link

codecov-io commented Apr 6, 2020

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #10972   +/-   ##
=======================================
  Coverage   43.38%   43.38%           
=======================================
  Files         597      597           
  Lines       84526    84526           
=======================================
+ Hits        36668    36675    +7     
+ Misses      43329    43327    -2     
+ Partials     4529     4524    -5     
Impacted Files Coverage Δ
models/repo_mirror.go 2.12% <0.00%> (-10.64%) ⬇️
modules/queue/workerpool.go 56.93% <0.00%> (-1.07%) ⬇️
services/pull/pull.go 35.00% <0.00%> (-0.89%) ⬇️
services/mirror/mirror.go 18.75% <0.00%> (-0.66%) ⬇️
modules/git/repo.go 48.11% <0.00%> (+0.83%) ⬆️
modules/log/event.go 65.64% <0.00%> (+1.02%) ⬆️
services/pull/check.go 50.00% <0.00%> (+2.43%) ⬆️
modules/git/utils.go 70.14% <0.00%> (+4.47%) ⬆️
modules/indexer/stats/db.go 59.37% <0.00%> (+18.75%) ⬆️
modules/indexer/stats/queue.go 81.25% <0.00%> (+18.75%) ⬆️

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 ef89e75...bf08400. Read the comment docs.

@@ -139,7 +139,7 @@ func testNewIssue(t *testing.T, session *TestSession, user, repo, title, content
htmlDoc = NewHTMLParser(t, resp.Body)
val := htmlDoc.doc.Find("#issue-title").Text()
assert.Equal(t, title, val)
val = htmlDoc.doc.Find(".comment-list .comments .comment .render-content p").First().Text()
Copy link
Contributor Author

@Sorien Sorien Apr 6, 2020

Choose a reason for hiding this comment

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

thx for updates, I removed this class couse it's setting max-width: 650px; (semantic) later it was overridden with gittea styles to 100% but it's not correct, but it seems that max-width: inherit; is working as expected so I'll recheck in few more browsers and reintroduce that comments class

IE/Edge sucks :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Thats ok imo we aren't really using the comments class here as it is intended by fomantic since this is more of a timeline of various events than a comment response thread on a blog

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fomantic can be customized by https://fomantic-ui.com/usage/theming.html so we should be able to override this variable like Sorien@7afe937 to my knowledge it should be working but it's not :/

Copy link
Member

Choose a reason for hiding this comment

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

Fomantic CSS is currently not being custom-built but copied because of a outstanding issue, see #10655. We'll be able to custom-build again with their next release, but still waiting on it.

Copy link
Member

@silverwind silverwind Apr 10, 2020

Choose a reason for hiding this comment

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

Or to be slightly more correct: Custom build does run but we copy the full CSS file over the custom version with that workaround. JS is still kept as custom build.

@lunny lunny added the topic/ui Change the appearance of the Gitea UI label Apr 6, 2020
@6543
Copy link
Member

6543 commented Apr 6, 2020

Screenshot at 2020-04-06 16-44-01

Can you fix the dark-theme too :)

@Sorien Sorien requested review from 6543 and mrsdizzie April 6, 2020 15:41
@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 Apr 6, 2020
@lafriks lafriks added this to the 1.12.0 milestone Apr 6, 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.

+1

@Sorien
Copy link
Contributor Author

Sorien commented Apr 7, 2020

I'd like to override some default Fomantic-UI variables. I have tried it according to their website but nothing works :/ ... is there anybody responsible for UI?

@silverwind
Copy link
Member

Unfortunately, we can not currently build custom fomantic CSS because of workaround #10655. I plan to remove it with the next fomantic release that includes the very important fix fomantic/Fomantic-UI@7fd3f79.

@silverwind
Copy link
Member

I guess we could runtime-patch fomantic with fomantic/Fomantic-UI@7fd3f79 if it's really important.

@Sorien
Copy link
Contributor Author

Sorien commented Apr 10, 2020

@silverwind it is not so important for this pr but removing comments class can lead to unwanted visual changes (so far I noticed color changes of comment's header links (buttons) at right, that was fixed) but it could be more...

I have prepared some other changes that are counting with building custom Fomantic

</div>
<div class="ui attached segment">
<div class="detail">
<span class="text grey has-emoji">{{.Content}}</span>
Copy link
Member

Choose a reason for hiding this comment

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

This should be black, else all review comments are grey

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is much more such things, like 4 different styles for author's name

Copy link
Member

Choose a reason for hiding this comment

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

I don't think I understand, this styling is just for the content not the author name

Copy link
Contributor Author

@Sorien Sorien Apr 10, 2020

Choose a reason for hiding this comment

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

Is it something that is changed by this PR? it was just copied, as you can see here https://github.com/go-gitea/gitea/pull/10972/files/bf084000712e45eb4c1b92bb5eac213634f8f57c#diff-7b9c057e968c3c88205ca2b63f0bfbfbL351

I was just pointing that there are inconsistencies all over the place...

Copy link
Member

Choose a reason for hiding this comment

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

It was grey before, but it was also displayed in the timeline before and not as its own comment. I think displaying it as a comment it should have black text like other comments.

Since this PR is about making lots of changes to the look it is ok to change things from how they were before if you think they will look better another way. If you think something seems bad or wrong feel free to make/suggest a change

Copy link
Contributor Author

@Sorien Sorien Apr 10, 2020

Choose a reason for hiding this comment

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

I'm afk for a few days, if you can fix it, simply, by removing grey class pls push to this branch you should still have rights

I'll try to fix some more things in further PRs

@mrsdizzie
Copy link
Member

I think it is ok to not use the fomantic comments class to contain everything here-- it actually does very little itself:

https://github.com/fomantic/Fomantic-UI/blob/662c43f82de2b11677c1a7dd7fa5052181b4cdc5/dist/components/comment.css

And we are already heavily modifying individual comments/avatars css and not using other intended classes like author and metadata so I don't think it matters to ditch comments here as long as things look fine (and I think our timeline is a bit different than a collection of comments like on a blog which is the fomantic example: https://fomantic-ui.com/views/comment.html).

I think timeline-items is more descriptive as well

Copy link
Member

@guillep2k guillep2k left a comment

Choose a reason for hiding this comment

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

Excellent job!

@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 Apr 10, 2020
@lafriks lafriks merged commit c97e988 into go-gitea:master Apr 10, 2020
a1012112796 added a commit to a1012112796/gitea that referenced this pull request Apr 11, 2020
fix wrong conditions check in initIssueComments after go-gitea#10972
guillep2k added a commit that referenced this pull request Apr 11, 2020
…1040)

* fix some bug about Request review

* fix ``CreateComment`` wrong using ,it will not work when use Sqlite
* fix wrong js click event code , it will send wrong data when it has
many choices

Signed-off-by: a1012112796 <1012112796@qq.com>

* Apply suggestions from code review

Co-Authored-By: Lauris BH <lauris@nix.lv>

* add getReviewerByIssueIDAndUserID
fix wrong conditions check in initIssueComments after #10972

* call CI again

Co-authored-by: Lauris BH <lauris@nix.lv>
Co-authored-by: techknowlogick <techknowlogick@gitea.io>
Co-authored-by: guillep2k <18600385+guillep2k@users.noreply.github.com>
ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
Co-authored-by: mrsdizzie <info@mrsdizzie.com>
ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
…-gitea#11040)

* fix some bug about Request review

* fix ``CreateComment`` wrong using ,it will not work when use Sqlite
* fix wrong js click event code , it will send wrong data when it has
many choices

Signed-off-by: a1012112796 <1012112796@qq.com>

* Apply suggestions from code review

Co-Authored-By: Lauris BH <lauris@nix.lv>

* add getReviewerByIssueIDAndUserID
fix wrong conditions check in initIssueComments after go-gitea#10972

* call CI again

Co-authored-by: Lauris BH <lauris@nix.lv>
Co-authored-by: techknowlogick <techknowlogick@gitea.io>
Co-authored-by: guillep2k <18600385+guillep2k@users.noreply.github.com>
@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.

10 participants