-
Notifications
You must be signed in to change notification settings - Fork 189
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
Complete Gatsby MaterialUI installation plus 'Add Feeds' page wireframing #735
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
import React from 'react'; | ||
import { Container, Box, Typography, TextField, Grid, Card, IconButton } from '@material-ui/core'; | ||
import { AccountCircle, RssFeed, HelpOutline, Add } from '@material-ui/icons'; | ||
import { makeStyles } from '@material-ui/core/styles'; | ||
|
||
const useStyles = makeStyles(theme => ({ | ||
margin: { | ||
margin: theme.spacing(2), | ||
}, | ||
button: { | ||
padding: '3px 0 3px 0', | ||
}, | ||
})); | ||
|
||
export default function MyFeeds() { | ||
const classes = useStyles(); | ||
return ( | ||
<div className={classes.margin}> | ||
<Container maxWidth="xs" bgcolor="aliceblue"> | ||
<Card> | ||
<Box px={2} py={1}> | ||
<Typography variant="h3" component="h3" align="center"> | ||
My Feeds | ||
</Typography> | ||
<Grid container spacing={5}> | ||
<Grid item> | ||
<Grid container spacing={1} alignItems="flex-end"> | ||
<Grid item> | ||
<AccountCircle /> | ||
</Grid> | ||
<Grid item> | ||
<TextField id="author" label="John Doe" disabled /> | ||
</Grid> | ||
</Grid> | ||
</Grid> | ||
<Grid item> | ||
<Grid container spacing={1} alignItems="flex-end"> | ||
<Grid item> | ||
<RssFeed /> | ||
</Grid> | ||
<Grid item> | ||
<TextField id="url" label="Blog feed URL" /> | ||
</Grid> | ||
<Grid item> | ||
<IconButton color="primary" classes={{ root: classes.button }}> | ||
<HelpOutline /> | ||
</IconButton> | ||
</Grid> | ||
</Grid> | ||
<Grid container spacing={2}> | ||
<Grid item> | ||
<IconButton classes={{ root: classes.button }}> | ||
<Add /> | ||
</IconButton> | ||
</Grid> | ||
</Grid> | ||
</Grid> | ||
</Grid> | ||
</Box> | ||
</Card> | ||
</Container> | ||
</div> | ||
); | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Do we want to keep styles located in the react component? Is there a way to store this in the css file that should be located with the page to keep with the current format of the front end?
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.
Yes, it is technically possible to replace this instance of
makeStyles
with pure CSS, i.e.Relying more on MaterialUI's styling solutions may become more useful in the future if/when we want to incorporate more MUI functionalities, such as customized theming.
However, since I'm still pretty new to MUI, I'd like to defer to @miggs125 and @humphd regarding whether it would be better, in this case, to embrace or move away from MUI styling solutions.
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 agree - not sure if having each react component contain our materialUI solutions or if we should grab it from a separate page that would house all the material UI.
I do think we should keep anything CSS related outside of the functionality part. Perhaps we could import it and have a different file such as myFeedsCss.js? (or materials not sure what to call this file)
This would allow the CSS developers to quickly locate where the CSS is that would need to be changed, and not get them stuck in reading everything within a component?
Thoughts @humphd ?
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.
Depending on the outcome of #730, it looks like we might use MUI as a base. If we do, then I think moving toward the MUI style approach is nice, because it takes less work to translate what their examples in the docs to to what we'll have. Doing CSS-in-JS this way is kind of nice.
I think do whatever you want in this PR, and we'll change it to match #730. I'll add a note about this topic there.