Skip to content

Conversation

@LiteSun
Copy link
Contributor

@LiteSun LiteSun commented May 4, 2021

This PR has made some corrections to the style issue.

  • Reposition the search button in the input box.
  • Adjust the Merge button to the top right of the chart.
  • Render shadows when only one repo is available
  • Reduce the size of the chart.
  • Adjust the page gap to make the page more compact.

current style:

image

new style:

image

@vercel
Copy link

vercel bot commented May 4, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/apiseven/contributor-graph/GGYu3KAkXVh66MuZFLi71GEjVfs8
✅ Preview: https://contributor-graph-git-improve-style-apiseven.vercel.app

@LiteSun LiteSun requested a review from juzhiyuan May 4, 2021 03:34
@juzhiyuan juzhiyuan changed the title feat: Improve page style style: improve page style May 4, 2021
@juzhiyuan
Copy link
Contributor

image

image

Users may don't know what's Merge View, we'd better add tips on this button.

  1. When choosing Monthly View, I notice there have some conflicts contents.

image

  1. Do we need those spaces?
    image

@Yiyiyimu
Copy link
Contributor

Yiyiyimu commented May 6, 2021

I think we'd better move markdown link outside the general share button, or users would easily missed it

@LiteSun
Copy link
Contributor Author

LiteSun commented May 6, 2021

I think we'd better move markdown link outside the general share button, or users would easily missed it

IMO, The markdown link is also a shared content and I think the same content should be put together🤔🤔. Waiting for other's views

@juzhiyuan
Copy link
Contributor

You may request more reviewers..

@Yiyiyimu Yiyiyimu requested review from membphis and moonming May 6, 2021 02:00
@moonming
Copy link

moonming commented May 6, 2021

Is there an online demo?

@Yiyiyimu
Copy link
Contributor

Yiyiyimu commented May 6, 2021

@juzhiyuan
Copy link
Contributor

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/apiseven/contributor-graph/CSeKUyLBbRs1veiGq1VoKSEzbMNQ
✅ Preview: https://contributor-graph-git-improve-style-apiseven.vercel.app

Here

@moonming
Copy link

moonming commented May 6, 2021

  • The search box is too long
  • The comparison function is not obvious
  • Remove download button
  • Enhanced sharing function
  • All contributors to Apache APISIX repositories are displayed by default

@LiteSun
Copy link
Contributor Author

LiteSun commented May 6, 2021

  • All contributors to Apache APISIX repositories are displayed by default

Is it a new feature request? Currently the default display is apache/apisix for one repository

@Yiyiyimu
Copy link
Contributor

Yiyiyimu commented May 7, 2021

Legend font size is still too small for README

image

@LiteSun
Copy link
Contributor Author

LiteSun commented May 12, 2021

image

image

Users may don't know what's Merge View, we'd better add tips on this button.

  1. When choosing Monthly View, I notice there have some conflicts contents.

image

  1. Do we need those spaces?
    image

updated.

@LiteSun
Copy link
Contributor Author

LiteSun commented May 12, 2021

I think we'd better move markdown link outside the general share button, or users would easily missed it

updated.

@juzhiyuan
Copy link
Contributor

Hi, the previous issues I mentioned have been resolved, but I found some new issues.

  1. Just to make sure, if the Merge button only works for APISIX repo?
  2. The Share URL is too long... If I have 3+ repos, it's too long.
  3. See https://contributor-graph-api.apiseven.com/contributors-svg?chart=contributorOverTime&repo=30-seconds/30-seconds-of-code,apache/apisix
    image

It's ok for me to discuss those questions or issues in another PR.

@juzhiyuan juzhiyuan merged commit 9f776f9 into master May 12, 2021
@juzhiyuan juzhiyuan deleted the improve-style branch May 12, 2021 01:05
@Yiyiyimu
Copy link
Contributor

@LiteSun need to merge to the download PR

@LiteSun
Copy link
Contributor Author

LiteSun commented May 18, 2021

@LiteSun need to merge to the download PR

Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants