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

Add Profile Page #17

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

Add Profile Page #17

wants to merge 8 commits into from

Conversation

bsakhuja
Copy link
Member

@bsakhuja bsakhuja commented Feb 15, 2018

Progress towards adding a profile page.

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 cause existing functionality to work as expected)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Purpose

Approach

  • Adds the basic form (Name, DJ Name, Email Address, and Password) sans profile photo (and ability to upload photo) for a profile page.

Screenshot(s)

screen shot 2018-02-14 at 5 16 45 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.

@codecov
Copy link

codecov bot commented Feb 15, 2018

Codecov Report

Merging #17 into master will increase coverage by 7.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #17      +/-   ##
==========================================
+ Coverage    62.2%   69.23%   +7.02%     
==========================================
  Files          12       17       +5     
  Lines         127      156      +29     
  Branches       15       15              
==========================================
+ Hits           79      108      +29     
  Misses         48       48
Impacted Files Coverage Δ
src/Background/Background.tsx 92% <ø> (ø) ⬆️
src/Profile/ProfileSubmitButton.tsx 100% <100%> (ø)
src/Profile/ProfileFormTextInput.tsx 100% <100%> (ø)
src/Profile/ProfilePicture.tsx 100% <100%> (ø)
src/Profile/Profile.tsx 100% <100%> (ø) ⬆️
src/Profile/ProfileLabel.tsx 100% <100%> (ø)
src/Profile/ProfileForm.tsx 100% <100%> (ø)
... and 2 more

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 9589cf3...b1ad1af. Read the comment docs.

src/index.css Outdated
@@ -3,3 +3,23 @@ body {
padding: 0;
font-family: sans-serif;
}

.profile-form form {
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why you chose to put some styles in index.css and some directly in the React component?

Copy link
Member Author

Choose a reason for hiding this comment

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

For the form, I couldn't figure out a way to add transparency to the backgroundColor property. I don't think there was any reasoning behind the others. I could put them all in the React component if that's the best style.

borderRadius: 5,
});

const SubmitButton = glamorous.button({
Copy link
Member

Choose a reason for hiding this comment

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

@binerys what are your thoughts on having multiple styled components in the same file? I think I remember you saying we should only have one component/file but idk what's the best practice here.

Copy link
Member

@nathanmsmith nathanmsmith Feb 15, 2018

Choose a reason for hiding this comment

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

Oh hey s/o to JavaScript twitter, which led me to this StackOverflow post which I really like!

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I think I understand now. So each different styled component usually gets their own file?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think that'd be best

@nathanmsmith
Copy link
Member

This looks like a really good start, thanks @bsakhuja! Once you finish adding the image part too, I'd recommend looking into testing your components with Jest and Enzyme. Here's a little writeup on using it with Typescript-flavored Create-React-App.

I was also thinking it might be a good idea to add some type of component documentation/management thing like Storybook before too long to our project; what are your thoughts? Here's a relevant thread on options.

"name": "panel",
"version": "0.1.0",
"lockfileVersion": 1,
"requires": true,
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 not add a package-lock.json file, since yarn uses yarn.lock!

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, I'm not sure how that got file got added/generated.

@bsakhuja bsakhuja requested a review from binerys as a code owner March 2, 2018 02:48
src/Profile/ProfileSubmitButton.tsx Outdated Show resolved Hide resolved
src/Profile/ProfilePicture.tsx Outdated Show resolved Hide resolved
@nathanmsmith
Copy link
Member

Looks great! Can we center and make shrek circular?

@nathanmsmith
Copy link
Member

Also can we reduce the spacing between each input field too?

@bsakhuja
Copy link
Member Author

Updated screenshot
screen shot 2018-03-16 at 4 46 47 pm

Copy link

@accesslint accesslint bot left a comment

Choose a reason for hiding this comment

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

There are accessibility issues in these changes.

const shrek = require('./classy-shrek.png');

const ProfilePicture = () => (
<Img src={shrek} height="260" width="260" borderRadius="50%" />
Copy link

Choose a reason for hiding this comment

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

This image is missing a text alternative (alt attribute). This is a problem for people using screen readers.

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