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

feat: show more data (in an embeded way) #455

Merged
merged 20 commits into from
Oct 6, 2022

Conversation

tyn1998
Copy link
Member

@tyn1998 tyn1998 commented Sep 13, 2022

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

Related issues:

Details

This illustration shows what we want to achieve:

image

Subtasks:

  • star, fork
  • issue comment, open issue
  • op, mp, rc, ad, de
  • p
  • i18n support (necessary?)

Checklist

Others

@tyn1998
Copy link
Member Author

tyn1998 commented Sep 13, 2022

react-tooltip helps to implement the feature that when users hovering on certain elements there will be tooltips showing up and presenting anything we want.

image

@tyn1998
Copy link
Member Author

tyn1998 commented Sep 17, 2022

embeded views for star and fork come out:

light theme:

image

image

dark theme:

image

image

@tyn1998
Copy link
Member Author

tyn1998 commented Sep 17, 2022

Hi, @frank-zsy, I have some confusion about the data:

image

The json file:

image

I know the number of fork event is not equal to the number of fork, but how to explain the numbers show in the screenshot above? Why there is a huge gap?

Are unfork events counted into fork events?

@frank-zsy
Copy link
Contributor

So cool!!

@frank-zsy
Copy link
Contributor

@tyn1998 I think the data is not right, I will check it.

@frank-zsy
Copy link
Contributor

I see, I put the data in wrong key, right now, the data in f is actually star count, in s is participants and in p is fork count. You can still use these keys and I will fix the data.

@frank-zsy
Copy link
Contributor

And I think line chart can be used instead of bar chart which is more common in fork and star trending.

@frank-zsy
Copy link
Contributor

If you are going to use hover to trigger the charts, please make sure that the mouse cursor can easily enter the chart and keep the charts showing so users can dive into the data. This may involve enlarge the detection fields of each charts.

I am not sure how to implement it but I think I expressed what I want.

@tyn1998
Copy link
Member Author

tyn1998 commented Sep 17, 2022

I see, I put the data in wrong key, right now, the data in f is actually star count, in s is participants and in p is fork count. You can still use these keys and I will fix the data.

Hah~got it.

And I think line chart can be used instead of bar chart which is more common in fork and star trending.

emm, the value of each month represents increment (i.e. 300 fork events ≈ 300 new fork), right? I considered line chart first but turned to bar chart since I thought "trending" required total number rather icrement number. But I can add a line to existing bars so we can present them all in one chart.

If you are going to use hover to trigger the charts, please make sure that the mouse cursor can easily enter the chart and keep the charts showing so users can dive into the data. This may involve enlarge the detection fields of each charts.

Sure, I have considered this. The hover-triggered box can be entered.

@frank-zsy
Copy link
Contributor

The data update process should be completed by tomorrow morning and the data will be updated to 2022.8

@tyn1998
Copy link
Member Author

tyn1998 commented Oct 1, 2022

And I think line chart can be used instead of bar chart which is more common in fork and star trending.

Hi @frank-zsy, this is how a line chart looks like:

image

and its bar chart counterpart:

image

or a combination of both:

image

Which way do you prefer? I'm ok with all of them but mildly incline to the combination chart :) How about you guys? @will-ww @wxharry

BTW, if we adopt line charts, I need to insert "zero" data for those months without a single event because of:

image

@frank-zsy
Copy link
Contributor

I think combination version is better, but what I like is that use bar chart to show the increase value of each month and use line chart to show the accumulate value of all history.

@tyn1998
Copy link
Member Author

tyn1998 commented Oct 1, 2022

Ok, I will implement it and update my progress here~

@frank-zsy
Copy link
Contributor

But there is a problem that the accumulate value is not accurate since the data is from log events rather than API. So the users may feel quite confused and need to explain the difference. So we need to decide whether to add the accumulate line chart since it may cause extra cost of explanation.

wj23027 and others added 4 commits October 3, 2022 13:22
@tyn1998 tyn1998 force-pushed the feat/add-more-data branch from fac0f04 to c4ac0c8 Compare October 3, 2022 05:26
@tyn1998
Copy link
Member Author

tyn1998 commented Oct 3, 2022

For those months with 0 event, I insert zero data to make the line chart complete:

2022-10-03 13 27 34

@tyn1998
Copy link
Member Author

tyn1998 commented Oct 3, 2022

embeded view for issue details:

2022-10-03 22 23 16

@tyn1998
Copy link
Member Author

tyn1998 commented Oct 3, 2022

embeded view for PR details:

2022-10-03 23 39 33

@tyn1998
Copy link
Member Author

tyn1998 commented Oct 3, 2022

  • tooltip format: not show day since the minimal is month

image

@tyn1998
Copy link
Member Author

tyn1998 commented Oct 4, 2022

image

@tyn1998
Copy link
Member Author

tyn1998 commented Oct 4, 2022

Finish basic version:

add-more-data.mov-8mb.mp4

@tyn1998 tyn1998 changed the title WIP: feat: show more data (in an embeded way) feat: show more data (in an embeded way) Oct 4, 2022
@tyn1998 tyn1998 marked this pull request as ready for review October 4, 2022 17:07
@wxharry
Copy link
Collaborator

wxharry commented Oct 4, 2022

The injections look perfect! Really appreciate your work these days! The interactions are smooth and I don't have any sense of being interrupted.
We still need a uniform title for these charts though. Whether to use plural form or not. And the nanme in the legends might be oversimplified. Maybe we could compose a detailed explanation and refer to that document some day.
image
image
Again. Really appreciate your work Lam!

@tyn1998
Copy link
Member Author

tyn1998 commented Oct 5, 2022

@wxharry Thank your for yor appreciation!

We still need a uniform title for these charts though. Whether to use plural form or not.

Agree. Actually I tried, but it was so hard for me to decide on the use of plural forms. Could you help? Another limitation is that Echarts doesn't provide an option to center the title text so if users want to center the title text they should adjust left, top, etc. pixel by pixel, which is time consuming! Due to this, I tried to make the titles as long as the width of the charts to avoid trivial tuning, which might make the titles a little difficult to understand.

And the nanme in the legends might be oversimplified.

Agree. But they cannot be long since the width of the chart is limited. Docs are good and some hover-to-show features also make sense (support in future).

@wxharry
Copy link
Collaborator

wxharry commented Oct 5, 2022

Agree. Actually I tried, but it was so hard for me to decide on the use of plural forms. Could you help?

IMO, using plural form for all titiles is fine.

But they cannot be long since the width of the chart is limited.

Maybe use one word for each legend? Since we have a detailed title, it would be easy for users to understand what that is.

@wxharry
Copy link
Collaborator

wxharry commented Oct 5, 2022

Also, I noticed that there is a code frequency section in insight tab, which looks similiar to one of our graphs. Are these the same?
image

@tyn1998
Copy link
Member Author

tyn1998 commented Oct 6, 2022

Not the same. Our data only count code line changes after PR merge, while the GitHub counterpart might also include code line changes made by direct push.

Copy link
Collaborator

@wxharry wxharry left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member Author

@tyn1998 tyn1998 left a comment

Choose a reason for hiding this comment

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

LGTM.

Hi @wxharry, please review and approve :)

@wxharry
Copy link
Collaborator

wxharry commented Oct 6, 2022

/approve

@menbotics menbotics bot added the pull/approved If a pull is approved, it will be automatically merged label Oct 6, 2022
@menbotics menbotics bot merged commit 32edf7a into hypertrons:master Oct 6, 2022
@tyn1998 tyn1998 mentioned this pull request Nov 6, 2022
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pull/approved If a pull is approved, it will be automatically merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] embeded view for star and fork (s,f) [Feature] Add more data for hypercrx
4 participants