Skip to content
This repository has been archived by the owner on Jun 9, 2023. It is now read-only.

Thomasroest/basic layout component #159

Conversation

ThomasRoest
Copy link
Contributor

  • I have read Chapter's contributing guidelines.
  • My pull request has a descriptive title (not a vague title like Update README.md).
  • My pull request targets the master branch of Chapter.

Adding some structure with a basic layout component, header and footer

Copy link
Contributor

@xarielx xarielx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to keep a flexible grid that ensures consistency across layouts, containing details about how content reflows on different screens and providing ease of how an app can scale from small to extra-large screens. At this moment, after reviewing the UI that we have designed thus far, the simplicity that they have, which is the vision that we have, to keep simple, it is better to use the Material UI since it constraints styling to their existing guidelines. For example:
Inline-style:
alt text
image
Here is some helpful documentation: Grid

client/components/Layout/styles.ts Show resolved Hide resolved
@xarielx
Copy link
Contributor

xarielx commented Nov 16, 2019

I just finished my review, based on today's meeting and since our view is to keep the UI/UX as simple as possible, I believe that we should continue using Material UI moving forward since this provides an easy way for us to provide a uniform experience.

@xarielx xarielx closed this Nov 16, 2019
@ThomasRoest
Copy link
Contributor Author

I would rather see some suggestions on improving the code, instead of some vague statements about grids and pointing to docs. You don't need a grid for a simple single column layout. If you do need it later on ( for larger screen sizes) you can add it to the main component for example.

@Sonicrida
Copy link
Contributor

At the very least, we should let @ThomasRoest refactor to use grid if we think it needs to be in right now. I would prefer to have more actionable advice in code reviews vs closing a PR when at least in this case, it's not too much of a change to refactor if it is necessary.

I don't feel like the screenshots that have been posted demonstrate the need for a Grid component at this time.

@xarielx xarielx reopened this Nov 16, 2019
@xarielx
Copy link
Contributor

xarielx commented Nov 17, 2019

If we could include the Material UI layout in this pull request I can merge it. Thank you

@QuincyLarson
Copy link
Contributor

During our Nov 30 meeting, we determined we want to use Materialize UI for the sake of shipping our MVP sooner. This way, we will not have to spend as much time creating custom Flexbox code.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants