Skip to content
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 3 commits into from
Feb 21, 2020

Conversation

Silvyre
Copy link
Contributor

@Silvyre Silvyre commented Feb 17, 2020

Issue This PR Addresses

Addresses #730 - Pick a React component library as our base UI toolkit
Resolves #725 - Prototype the Add Feed view in React

Type of Change

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

Preview of 'My Feeds' component:

  • It is worth noting that this component currently ends up being rendered quite small due to styling enforced by src/frontend/src/pages/index.css:
    html {
    /*assuming browser font default is 16px*/
    font-size: 62.5%;
    /*translates to 10px -> root font*/
    }

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

import { AccountCircle, RssFeed, HelpOutline, Add } from '@material-ui/icons';
import { makeStyles } from '@material-ui/core/styles';

const useStyles = makeStyles(theme => ({
Copy link
Contributor

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?

Copy link
Contributor Author

@Silvyre Silvyre Feb 17, 2020

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.

.MuiContainer-root {
  position: relative;
  top: 16px;
}

.MuiIconButton-root {
  padding: 3px 0 3px 0;
}

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.

Copy link
Contributor

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 ?

Copy link
Contributor

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.

src/frontend/src/pages/myfeeds.js Outdated Show resolved Hide resolved
src/frontend/src/pages/myfeeds.js Outdated Show resolved Hide resolved
Copy link
Contributor

@Grommers00 Grommers00 left a comment

Choose a reason for hiding this comment

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

Looks good,

We can switch CSS later if we notice an issue or decide to go another route.

Copy link
Contributor

@Grommers00 Grommers00 left a comment

Choose a reason for hiding this comment

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

Can we also add another option for the main page to navigate here :D. Perhaps in the nav bar?

@Silvyre
Copy link
Contributor Author

Silvyre commented Feb 21, 2020

Can we also add another option for the main page to navigate here :D. Perhaps in the nav bar?

In my opinion, a higher priority would be to enable route guarding for this component. However, both of these functionalities would be more appropriately handled within separate issues.

@Grommers00
Copy link
Contributor

Can we also add another option for the main page to navigate here :D. Perhaps in the nav bar?

In my opinion, a higher priority would be better to enable route guarding for this component. However, both of these functionalities would be more appropriately handled within separate issues.

Agreed, However nav bar is rendered in a way that we only want this page shown in logged in mode anyways.

As I don't know if we are adding in protected routes this way..or necessarily how as we don't have a state manager given that it's a simple application having the link only show up when logged in the navbar I think could suffice?

@Silvyre
Copy link
Contributor Author

Silvyre commented Feb 21, 2020

Can we also add another option for the main page to navigate here :D. Perhaps in the nav bar?

In my opinion, a higher priority would be better to enable route guarding for this component. However, both of these functionalities would be more appropriately handled within separate issues.

Agreed, However nav bar is rendered in a way that we only want this page shown in logged in mode anyways.

As I don't know if we are adding in protected routes this way..or necessarily how as we don't have a state manager given that it's a simple application having the link only show up when logged in the navbar I think could suffice?

That's a good point, especially coupled with the fact that there is talk of designing Telescope as a single page site.

Copy link
Contributor

@cindyorangis cindyorangis left a comment

Choose a reason for hiding this comment

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

This looks good. CSS can be filed as another issue or if it needs to be discussed some more, it can be done in the frontend discussion. You can bug @agarcia-caicedo about the navbar in #588

@Silvyre Silvyre requested a review from Grommers00 February 21, 2020 14:00
@Silvyre Silvyre merged commit d9a37d1 into Seneca-CDOT:master Feb 21, 2020
@Silvyre Silvyre deleted the add-feeds branch February 21, 2020 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: design Issues needing design or assets area: front-end
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prototype the Add Feed view in React
4 participants