Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Main Page Begins #21

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Main Page Begins #21

wants to merge 12 commits into from

Conversation

haejinjo
Copy link

@haejinjo haejinjo commented Mar 2, 2018

Note to peers

❗Please please point out anything that doesn't make sense, won't be scalable, doesn't take advantage of the frameworks at my disposal, or is just ugly in your opinion. #roastme because I want to know what I'm doing.

Types of changes

  • Bugfix (change which fixes an issue)
  • New feature (change which adds functionality)
  • Refactor (change which changes the codebase without affecting its external behavior)
  • Non-breaking change (fix or feature that would causes existing functionality to work as expected)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Purpose

Create the main page for panel following login screen.

Learning

Still trying to understand components and props in React. Used egghead.io videos.
Considered using flexbox react addon, but discussed with Nathan about using css grid instead. Thanks to Wes Bos and MDN for engaging CSS Grid tut

Screenshot(s)

screen shot 2018-03-01 at 10 40 36 pm

Checklist

  • My branch follows the branch naming scheme of UCLA Radio, and can merge into master without error.
  • My code follows the code style of this project, and I have linted it to confirm this.
  • I have added tests that prove my fix is effective or that my feature works.
  • My change requires a change to the documentation and I have updated the documentation accordingly.
  • All new and existing tests passed.

padding: 3,
});

const RectangleThing = glamorous.div({
Copy link
Member

Choose a reason for hiding this comment

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

Let's move this component into a separate file. I think maybe a more descriptive name than RectangleThing might also be a good idea--perhaps PageLink, since that's what each rectangle will contain?

src/index.js Outdated
@@ -0,0 +1,9 @@
import React from 'react';
Copy link
Member

Choose a reason for hiding this comment

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

We don't need this file, you can delete it!

<Div
className="link-grid"
display="grid"
grid-template-columns="repeat(3,400px)"
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Do you think inline styles like this are ok? I know we're using glamorous, but I've thinking of starting a css file for Home because I felt that was cleaner. What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

or is that defeating the purpose of using glamorous

backgroundColor: 'rgba(255, 255, 255, 0.8)',
});

// const LinkGrid = glamorous.div({
Copy link
Member

Choose a reason for hiding this comment

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

Remove comments if we aren't using them!

}
}

ReactDOM.render(<Home />, document.getElementById('root'));
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't be rendering DOM elements here, that's what the index file is for. Instead make sure this component included in the component!

Copy link
Author

Choose a reason for hiding this comment

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

Oh I see. So index renders App which renders all the page components like Home, Profile, Shows, etc?

Copy link
Member

Choose a reason for hiding this comment

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

yup!

Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend checking out App.tsx and index.tsx to see how this works! index.tsx is the file that is compiled behind the scenes.

@codecov
Copy link

codecov bot commented Mar 9, 2018

Codecov Report

Merging #21 into master will increase coverage by 5.11%.
The diff coverage is 93.1%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #21      +/-   ##
==========================================
+ Coverage    62.2%   67.32%   +5.11%     
==========================================
  Files          12       14       +2     
  Lines         127      153      +26     
  Branches       15       18       +3     
==========================================
+ Hits           79      103      +24     
- Misses         48       50       +2
Impacted Files Coverage Δ
src/Home/CircleButton.tsx 100% <100%> (ø)
src/Home/PageCard.tsx 100% <100%> (ø)
src/Home/Home.tsx 90.9% <90.47%> (-9.1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d3ae1d0...7ee77ca. Read the comment docs.

<meta name="viewport" content="width=device-width, initial-scale=1, shrink-to-fit=no">
<meta name="theme-color" content="#000000">
<!--
<html lang="en" style="max-width:100%; overflow-x:hidden;">
Copy link
Member

Choose a reason for hiding this comment

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

Why are adding these attributes to the html and body tags?

Copy link
Author

Choose a reason for hiding this comment

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

It's a fix I found online to prevent horizontal scrolling!

}
}

// ReactDOM.render(<Home />, document.getElementById('root'));
Copy link
Member

Choose a reason for hiding this comment

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

Remove comment

grid-column-gap="30px"
padding="50px"
>
{/* {
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove this comment for now, we'll come back to dynamically populating the page soon.

render() {
return (
<Div margin="5px auto" padding="20px" width="100%">
<Div id="nav" display="flex">
Copy link
Member

Choose a reason for hiding this comment

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

You can use the semantic nav element here instead of a div!

<Div
className="buttonGroup"
display="grid"
grid-template-columns="5vw 5vw 5vw"
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure vw is the best unit to use here. Isn't fr usually used for grids?

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.

3 participants