-
Notifications
You must be signed in to change notification settings - Fork 0
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
Homescreen and Infoscreen #11
Conversation
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.
Good overall, but I have some requests. Please address them and re-request my review once you're done. And don't forget to resolve the merge conflicts. If you have any questions, please don't mind asking!
left: 0; | ||
bottom: 0; | ||
right:0; | ||
width:100vw; |
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.
Remove these properties as they'd hide the navbar and anything else preceding it. Keep in mind that you're using position: absolute
here.
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.
I didn't find away to use the "remaining" height of the screensize. Therefore I had to stick to absolute positioning and bottom: 0. To make the navbar visible the homescreen component is now offset to the background. It will take 100% of the height and width all the time but no more. As I think the main screen of this application will remain quite clean I think it should be alright that way.
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.
You could have used the flexbox algorithm on the element enclosing the navbar and the homescreen component, but moving the homescreen component to the background should be just fine for now. We might address this in a future PR, however...
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.
Overall good improvement, but please address the issues that are still open.
Would also be great if you could use a proper formatter in the SCSS files (mind the missing space character between the colons and the properties' actual values).
left: 0; | ||
bottom: 0; | ||
right:0; | ||
width:100vw; |
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.
You could have used the flexbox algorithm on the element enclosing the navbar and the homescreen component, but moving the homescreen component to the background should be just fine for now. We might address this in a future PR, however...
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.
I'm really sorry for bothering you again, but I still want you to do two minor things:
- Please restore the original
package.json
(The items are rearranged onnpm install
s) - Remove the redundant
package-lock.json
in the project's root.
I promise, you'll be good to go then 👼
Finally, WOOHOO |
This PR adds the Homescreen and the Infoscreen for the website. The mdx "code" to test it lies in the content file "contentPage". The filter from the Homescreen is removed, but I work on getting it implemented again. On the info pages, the additional Infos are, other than we said earlier, aside the picture instead of under it. I think like that its more clear, where it belongs to.