-
Notifications
You must be signed in to change notification settings - Fork 7
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
Website layout #69
Website layout #69
Conversation
…nto website-layout
… into website-layout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yoonieaj @sarahsonder this is a good start, but you should not have created new components. Instead, you should modify the existing components, as they already contain the required functionality (and testing).
Pull Request Test Coverage Report for Build 10447705512Details
💛 - Coveralls |
… into website-layout
Hi Professor Liu, below are some notes and questions about this round of changes. Notes Testing changes: For tests involving Menu components, I've added a New files: When importing the logo, I ran into two problems.
The new files Questions Input Layout: Since we tried our best to follow the existing structure of the code, we decided not to change the layout of the input in this update. However, we are wondering if we could move the Menu Components: There are curently two Menu components, one in |
@yoonieaj and @sarahsonder good work; I'll do a full review soon, but in response to your immediate questions:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yoonieaj and @sarahsonder okay this is looking better. I left some review comments.
I'll highlight one general point: because we are using a UI component library, we should use custom CSS sparingly. It appears that you've been trying to use CSS to achieve a specific look that matches your initial design, which is understandable but creates extra work because you're adding new custom CSS that is finicky and would need to be maintained over time. I didn't comment everywhere you used custom CSS, but for your next iteration you should try to remove as much custom CSS as possible so that we allow the mui library's defaults to kick in and set the look of our website, while still achieving the overall goals of this pull request (modifying the layout, adding the logo).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yoonieaj and @sarahsonder I accidentally ended a review too early and went for lunch, which is why you'll see this second review (focused mainly on layout).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sarahsonder and @yoonieaj okay looking good. I left a few last minor comments. Also, please update the pull request description with final screenshots, and make sure you fill out the checklist.
Proposed Changes
Changed the website design to have a horizontal layout and to include the MemoryViz logo.
Website screenshot
Type of Change
Checklist
Before opening your pull request:
After opening your pull request: