Closed digitalimplementer closed 8 months ago
The latest updates on your projects. Learn more about Vercel for Git ↗︎
Name | Status | Preview | Updated (UTC) |
---|---|---|---|
joystream-org | ✅ Ready (Inspect) | Visit Preview | Feb 29, 2024 0:05am |
joystream-org-storybook | ✅ Ready (Inspect) | Visit Preview | Feb 29, 2024 0:05am |
joystream-org-translation | ✅ Ready (Inspect) | Visit Preview | Feb 29, 2024 0:05am |
Does not look right.
When working on token section I sticked to mobile-first and bem approaches. I ensured the markup is semantic and code is reusable. For instance, I created reusable components to represent info in widgets, tables, etc. To simplify styling of components I used such SCSS features as variables, mixins, functions (created those in a separate directory (styles/layout/dashboard). I ensured components have prop types specified. To implement complex features I used libraries that're already present in the project like recharts, react-responsive, pure-react-carousel. Token price chart includes area and bar charts combined. They have common x axis and separate y axises (bar chart's y axis is hidden) to represent info properly. The chart has custom active dot (active dot is represented by circle and line svgs, line is equal to chart's width and needed to intersect with tooltip's vertical line) and tooltip components. As for token release schedule chart, it is an area chart that has static reference line and custom legend in addition. As for token minting chart, it is a pie chart with custom labels and legend. It has support of mouseenter and mouseleave events applying conditional styling using classnames library. All charts have custom styling, functions to format tick labels, custom labels, tooltips, cursors, etc. As for the carousel, it has different quantity of visible slides specified according to media rules. It also has logic implemented to show or hide buttons to change slides. And the last but not least section feature is a table. I made it reusable so that it accepts columns (column must have accessor key specified), data and a classname to adjust styling if needed (it's populated than to table with classnames library) as props. Look forward to your feedback.
@DzhideX are you able to perform a code review at this stage, as quite a lot has already been done?
@msmadeline are you able to provide a preliminary design review of the implementation of the website so far, compared to the final designs: https://github.com/Joystream/joystream-org/issues/650#issuecomment-1845910654
Remember to check responsiveness across all screen sizes also.
Here is an example of an excellent detailed review from @KubaMikolajczyk
https://github.com/Joystream/joystream-org/pull/713#issuecomment-1815016413
Hey @DzhideX! Thank you for taking the time to review my PR. Your detailed feedback is incredibly valuable. I've planned to address your suggestions as follows: I am going to check the recharts responsive container behavior in terms of responsiveness as I think it causes the issue, then I am going to change the structure of dashboard-related components so that they are gathered in a single directory and after completing next sections I think I'll be able to take on translations. Thank you again for your time and expertise!
After doing some review, I think its clear we need to revise the design of the exchange section, but we can do it as a last item, description here: https://github.com/Joystream/joystream-org/issues/776
Hey @digitalimplementer the implementation looks great!⭐ Nice job. Please have in mind that I did the review yesterday/today and I know that you are currently fixing some things so If any of my points is already fixed - don't worry about it.
Hey @msmadeline! I truly appreciate you taking the time to review in detail my pull request. I am going to address the issues you indicated. Thank you again for your time and expertise!
Hi @DzhideX! The code of "Backers" and "History" sections is located in /components/DashboardBackers and /components/DashboardHistory directories accordingly. I bear in mind your proposal to restructure dashboard-specific components so I'll be gradually moving the code in /components/dashboard-page directory. I've also addressed previously specified issues with horizontal overflow.
Hi @msmadeline! Apart from implementing "Backers" and "History" sections I've also addressed the issues you specified. Moreover, I've made "Release schedule" chart interactive. The only issues specified I have doubts about are position of an active chart's dot on mobile devices that does not correspond to the position of tap and values of x axis of token release chart not being dynamic. As for the first one, the position of tap is represented by tooltip and an active dot vertical position will vary depending corresponding to Y axis value. As for the second one, the values are dynamic. In our case we have months from 0 to 29 and a function to format ticks, as per figma I specified the x axis to show only numbers divisibe by 5. Let me know if I am missing anything here!
Hey @digitalimplementer amazing implementation! Very impressed, the list of issues is pretty short⭐
When it comes to my previous feedback
Hi @DzhideX! Bearing in mind your proposal to restructure dashboard-specific components I've created dashboard-page directory in /components and placed the code of "Traction", "Engineering" and "Community" sections there. If you confirm the approach is okay I'll gradually move previously created components there.
Hi @msmadeline! I've addressed previosuly specified issues. The only proposal I've not yet addressed is the image height ("History" section) due to the fact to change image's height and make it fill in the container I need to change its ascpet-ratio. Thus, image's quality gets worse. Please let me know if this should be done anyway so that you can take a look. As for your request to clarify the issue with an active chart's dot on mobile devices not corresponding to the position of tap ("Token Price" chart), my point is that an active dot would be as high as its corresponding Y axis value (screenshot) and tooltip would reflect the tap (or click) position. Please let me know if I am missing anything here!
@bedeho
Stage | Estimated, h |
---|---|
Exchange | 8 |
Team | 12 |
Community | 12 |
Comparison | 8 |
Roadmap | 6 |
Possible fixes | 16 |
Hey @digitalimplementer Nice job 👍I found a couple of issues
There is an issue with all of the widgets form the traction section as they should fill the container, also something is wrong with the bottom padding inside the widgets with charts - I think the charts overlap the bottom padding and it creates less space from the bottom. Remember that charts shoulld also fill it’s container and with that said the size of the columns should adjust
There is a wrong gap between larger sections - it should be 80px
There is a title missing in the first widget of the Engineering section - “Github stats”
Something went wrong with the image in the DAO daily standup card - it should be visible from the left side
Gap between community cards should be 16px
The number of the contributors next to the section title is missing
the same issues as in the previous bp
The same issue with the gap between all of the big sections - should be 96px
The same issues as in previous bp's
The text size of the data inside of the traction section widgets should be 42px instead of 55px
The same issue with the gap between the big sections - should be 128px
The same issues as in previous bp's
There is a wrong layout of the widgets in traction section. There should be 2 widgets in one row instead of all of them just placed vertically
The same issue with the gap between the big sections - should be 160px
The same issues as in previous bp's
The gap between big sections should be 160 px
Hey @DzhideX! Following your suggestion, I have relocated the code for the page to the dedicated "dashboard-page" directory. So, the code for "Team", "Comparison" and "Roadmap" sections can be found there (e.g., /components/dashboard-page/Team). I've also ensured that the previously identified issues have been addressed. With ui being complete for the page I am moving on to api integration.
Hey @msmadeline! I've ensured that the previously identified issues have been addressed. For the "Roadmap" section I've leveraged the data currently presented on the dedicated /roadmap page to implement the functionality outlined in the "User stories".
Hey @digitalimplementer Incredible job! ⭐ Very impressed. Found only few issues.
Here's the updated copy for all of the text on the page (let me know if I've missed anything) @digitalimplementer:
Do we want the gleev link at top middle of page to link to gleev?
No, actually I don't know why that even made it on the page.
Do we want the button in the top right to be a discord chat widget or just redirect to our discord server?
No, lets remove it, we don't have an obvious solution
Where do we get the content for the history section?
No, actually we don't. Lets hide for first release.
What values do we want to use for the Comparison section? Is FDV ok atm?
Yes, FDV is fine.
Can we do this as well?
https://github.com/Joystream/joystream-org/issues/764#issuecomment-1937045704
Can we do this as well?
I'm sure that we can add this feature.
Hey @bedeho @digitalimplementer @DzhideX My part is done. I've designed a version of the dashboard with all of the widgets in a loading state. I displayed it for you in a single user story but basically we need to take a "Skeleton loader" component, make it a fixed proper size and replace any widget/card that we want with it. It is also animated
Also I've adjusted the larger breakpoints for header ✅
Hey @digitalimplementer Nice job , these are some issues that I found
On this breakpoint there’s no need for the placement card
In the minting widget - the numbers are slightly going out of the widget, maybe they should be a little smaller like 12px
Wrong text size of the procentage data in the supply staked for validation and APR on staking cards - should be 42px
In the videos uploaded widget there is the same issue with numbers going beyond the widget. Maybe we should align numbers to the left
till weird spacing between numbers under the contributions widget
In the councl card the “View past councils “ button shuld be under the term length info - so aligned to right
Also there is overall wrong padding on the entire page - on the right side it's less than 32 px
There is wrong layout of the information in the enginenring card
(pasting from Discord)
Here are some final thoughts:
What link do we want to use in Jsgenesis section @bedeho?
Which link?
@bedeho I meant for the anchor element in the bottom left corner in this section (the "Read more ->" link):
ah ok, lets link to www.jsgenesis.com
Hey @DzhideX! I've addressed the issues you raised. As for "Comparison" table, as I see, there is no dynamic data to populate the table with, so I've just changed the value of Joystream's FDV and provided the skeleton for the section.
Hey @msmadeline! There are a couple of things I'd like to clarify:
daysToShow
parameter;padding-inline
is set to 16px at 425px, the layout breaks. So, I set padding-inline
to 16px from 435px;padding
issue at lg bp? It seems it's 32px (screenshot);margin-top: auto
for legend container;Thanks in advance for your time!
@digitalimplementer
Contributions" x-axis ticks are formatted according to Figma. To address the issue of large gaps between ticks, I added a value of "30" to the daysToShow parameter;
- Yes but arent they wrong still? There is a random "Feb" in the middle of it. which I think should be at the end of the line. If we can't do anything about the gap between, then I guess it's fine but maybe we can at least put the numbers in a right order? Correct me If my thinking is wrong
I'm not quite sure I understand, could you clarify what "placement cards" mean?
- I meant the "placeholder cards" sorry
"Exchange" options (xs bp) have 13.75px inline gap between data container and border; if padding-inline is set to 16px at 425px, the layout breaks. So, I set padding-inline to 16px from 435px;
- When it comes to the exchange cards. In the screenshot you can see that the content is going outside the card from the bottom of it - that's the issue there. The card have to hug the content properly
could you elaborate on padding issue at lg bp? It seems it's 32px (screenshot)
- Yes the lg padding seems fine now. Maybe something broke on my side before
since "Minting" chart is in a grid, it has the same height as "Token allocation" table. Increasing the height of the chart container seems not to be influencing the size of the chart, so to address the issue of large gap between the chart and the legend I set margin-top: auto for legend container;
- Ok I understand that, Overall it seems fine
could not reproduce the problem of "View past councils" button text wrapping after changing the position of button. Let me know please if the problem persists;
- The positioning of the button is good now but still there's a problem that the copy should be in one line
could you elaborate on "Comparison" table. Should I wait for you to change some text properties in the design file or just increase the font-size to 14px, for example?
- I though we just droped it. But If we keep it then I need to try it out first in figma. Please work on the columns I show on the screenshot. I think they should be a bit higher maybe like 20px height. Right now they are not huggin the data properly