Sparsh1212
commented
2 years ago Can you also attach the Demo video
Closed aritroCoder closed 2 years ago
Sparsh1212
commented
2 years ago Can you also attach the Demo video
aritroCoder
commented
2 years ago aritroCoder
commented
2 years ago Some high-level comments:
- The info appearing on the right looks like a repetition of the graphs on the left. It will be best to remove the textual info as all of that could be interpreted from the graphs.
- Since, this is the launcher page, it's fine to render the overall stats of GSoC. By that I mean to say, there's no need for a "Show Summary" button. Please remove that.
- Since we want to make the search distraction-free, so please remove the show summary button from the search too. It will be best to remove it completely from the code.
- Regarding the UI. Finally, I think, it will be best to only render the graphs. That being said, make sure you center the new graphs added. In the first row, center the twp graphs and increase their size (height and width) so that they take the max-width. In the second row, just center the organization's graph, and the size should be around 60-70% of the width.
- The UI is not mobile responsive. In the mobile view, it will be best if each graph can take the full width and we can render the in the form of columns.
Ok , thanks for your detailed review. I am working on your points
aritroCoder
commented
2 years ago I have made the required changes. Please check the video. If everything looks fine I will commit the code and re initiate the PR.
Sparsh1212
commented
2 years ago I have made the required changes. Please check the video. If everything looks fine I will commit the code and re initiate the PR.
2021-12-12.23-21-03.mp4
Good job on the suggestions. After seeing the demo video, I realized that if we individually display the graphs in 100% width even on the desktop view, they look awesome. So final UI changes will be:
Sparsh1212
commented
2 years ago @aritroCoder Any updates on this?
aritroCoder
commented
2 years ago I have made the required changes. Please check the video. If everything looks fine I will commit the code and re initiate the PR. 2021-12-12.23-21-03.mp4
Good job on the suggestions. After seeing the demo video, I realized that if we individually display the graphs in 100% width even on the desktop view, they look awesome. So final UI changes will be:
- Make each graph almost 90% width and increase the height a bit. So in total there will be a total of 3 rows in both Desktop and Mobile View.
- Increase the spacing between the rows of graphs.
- I was thinking to remove all the launcher data content and keep it a two liner. That being said, remove all the existing content present which starts from "A blazingly fast tool to .... bookmark your favourite organization" and replace it with these two lines:
- "A blazingly-fast tool to analyze all the organizations selected in Google Summer of Code."
- "Start analyzing now!"
Yes I am working on it, it will be ready by tommorow. Thank you!
aritroCoder
commented
2 years ago I have made the requested changes, please see them and give any feedback
Sparsh1212
commented
2 years ago 2021-12-13.22-33-56.mp4 I have made the requested changes, please see them and give any feedback
The UI looks excellent. I though observed one small problem with the graph caption text on the mobile screen. Observe the graph caption at the bottom of the graph, please decrease its font size for mobile view. Rest the UI looks perfect to me. Push your code after this suggested fix and then we'll have a final round of code review and then get set go!
aritroCoder
commented
2 years ago 2021-12-13.22-33-56.mp4 I have made the requested changes, please see them and give any feedback
The UI looks excellent. I though observed one small problem with the graph caption text on the mobile screen. Observe the graph caption at the bottom of the graph, please decrease its font size for mobile view. Rest the UI looks perfect to me. Push your code after this suggested fix and then we'll have a final round of code review and then get set go!
I came at a different kind of solution for this. React chartjs does not use any CSS, as it renders the chart as a image. So I have used a logic at the code where it will decide the chart font size at the time of loading the screen . As the users wont be changing their screen size, so I think it will not be a issue. Do give your feedback
Sparsh1212
commented
2 years ago 2021-12-13.22-33-56.mp4 I have made the requested changes, please see them and give any feedback
The UI looks excellent. I though observed one small problem with the graph caption text on the mobile screen. Observe the graph caption at the bottom of the graph, please decrease its font size for mobile view. Rest the UI looks perfect to me. Push your code after this suggested fix and then we'll have a final round of code review and then get set go!
I came at a different kind of solution for this. React chartjs does not use any CSS, as it renders the chart as a image. So I have used a logic at the code where it will decide the chart font size at the time of loading the screen . As the users wont be changing their screen size, so I think it will not be a issue. Do give your feedback
2021-12-13.23-46-18.mp4
The approach sounds so cool to me. Also, always don't hesitate to push your code whenever you make a change to your PR. Viewing the changed code also helps the reviewer to guess whether the solution has been implemented in the correct way or not. Otherwise, it will lead to unnecessary extra iterations. :)
aritroCoder
commented
2 years ago I have committed the changes to the PR
aritroCoder
commented
2 years ago Did you check the code? Please check and update your status. Thank you
Sparsh1212
commented
2 years ago Also, please rename the Chart file names with more descriptive names. Replace Company with Organization everywhere in the naming. Also, if it's easy for you then please try to group all the three graph files into a separate folder under components named as launcher. And there put all the files of LaunchingComponent. This ensures good modularity of the code.
aritroCoder
commented
2 years ago I have done all your requested changes. Please go through the code now and give your suggestions
Sparsh1212
commented
2 years ago Also, please take care of the merge conflict.
aritroCoder
commented
2 years ago Done :)
aritroCoder
commented
2 years ago If this commit is perfect, I will create the final video
Sparsh1212
commented
2 years ago If this commit is perfect, I will create the final video
Yeah! things look pretty good to me in the latest commit. Just upload a final video and if everything looks good, then we can go for a merge :)
aritroCoder
commented
2 years ago second video is for mobile view. I think the gap between the typewriter text and chart is too much if you want I can reduce that too
Sparsh1212
commented
2 years ago 2021-12-15.19-44-23.mp4 2021-12-15.19-45-00.mp4 second video is for mobile view. I think the gap between the typewriter text and chart is too much if you want I can reduce that too
Yes, that will be really good if you could reduce the gap. You can achieve that simply by decreasing the min-height of the animatedDiv I think. Make sure to decrease it for the mobile view as well. Also, it will be good if you can slightly increase the separation between the graphs. Don't increase it too much, just a bit.
Sparsh1212
commented
2 years ago I think after these two fixes, we are fully set to go for a merge.
aritroCoder
commented
2 years ago Sparsh1212
commented
2 years ago Just one last change. You don't need to reupload the video again after this. Make the min-height in the PC view to be 34vh instead of 54vh. That's it.
aritroCoder
commented
2 years ago Alright, I did that :)
Added graphs for different GSOC stats and also a side pane which contains different data related to all previous year GSOC