Move information to the sidebar, enforce max width #447
Conversation
| @@ -5,7 +5,7 @@ function PluginReadableInstalls({currentInstalls}) { | |||
| if (!currentInstalls) { | |||
| return <>No usage data available</>; | |||
| } | |||
| return <>{currentInstalls}</>; | |||
| return <>{new Intl.NumberFormat('en-US').format(currentInstalls)}</>; | |||
There was a problem hiding this comment.
I've never used this util. If you specify the format does it only do it for us style numbers? Is possible to let the browser pick the locale?
There was a problem hiding this comment.
If we omit the locale name, the browser locale will be used. But when formatting dates we are also using US locale and almost all the texts are in English, so I thought this is more consistent.
There was a problem hiding this comment.
Oh yea totally a good plan. I was just curious how it worked.
I think I started separating dates into its own components for that reason, so we could eventually fix it. I just didn't go far enough
There was a problem hiding this comment.
Looks reasonable. Especially de-emphasizing the (often outdated) maintainers list is a good idea.
The graph takes some getting used to, but I can see it being there to only show the trend, and it should be fine for that. Glad we got the y=0 integrated before this 😉
Curious why we're lot putting all the metadata to the side, in a "Current Release" section? (Except for ID, which applies to all releases. That one may be best above the tabs, next to the label.)
src/components/LineChart.jsx
Outdated
| const calculateMinMax = (data) => { | ||
| const maxValue = Math.max(...data); | ||
| // calculate a dynamic value to center the graph | ||
| const scaleDifference = Math.pow(10, maxValue.toString().length-1); | ||
| return { | ||
| min: 0, | ||
| max: parseInt((maxValue/scaleDifference)+1)*scaleDifference // plus 1 to add more space in the top | ||
| max: Math.ceil((maxValue/scaleDifference) + 0.25)*scaleDifference // plus 0.25 to add more space in the top |
There was a problem hiding this comment.
This can be done much more simply if we're getting rid of the Y axis labels, as the only thing this seems to accomplish having no weird (non-even, differently spaces) outlier as the max Y value.
Just use maxValue*1.2 or so as the max?
There was a problem hiding this comment.
Fixed in the last commit.
|
Thanks for the review request @timja ! |
I didn's see a strong reason to move those and NPM is also not putting this information in the sidebar. But it can be moved of course. If we'd that, where should the ID go? |
My suggestion would be to try below display name and above tabs. |
…into sidebar-layout
|
@daniel-beck I moved the version to the right; the ID is in a floating block that's either below the title or to the right, depending on ID length and screen size.
Currently the version number itself is used as a heading to some space. |
|
Given the approvals I'll merge this in 24 hours if there is no negative feedback. |
This PR
Most of these changes are inspired by NPM.