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

Redesign groundwork, homepage, search #870

Merged
merged 4 commits into from Jan 9, 2018

Conversation

Projects
None yet
3 participants
@seanyesmunt
Copy link
Member

seanyesmunt commented Dec 14, 2017

Redesign

This is mostly basic groundwork (clean up) + header/home changes. Ignore the incomplete stuff for now, I don't want the PR to be 5k changes. I will continue to move a lot around and discuss anything before something is decided.

There is a lot going on in this PR. Future PR's should be smaller/more concise feature sets

Component improvement goals

Avoid internal state
Avoid unnecessary HTML markup (<React.Fragment> ftw)
Props driven styling for components over adding classes (see src/renderer/component/link)

Style improvement goals

BEM styling used across the app
Add basic style guide with colors, padding sizes, headers, buttons, philosophy (ex. Always prefer {margin,padding} {top,left} over {bottom,right})

Flow

I've (partially) added flow in a lot of places, I think I would like to come back at the end and spend some time just cleaning it up/ensuring everything looks good. Right now I'm mostly just getting the flow checker to pass in components I add it to. Ex: Adding a claim type as claim: { uri: string } if that is the only value used, instead of creating a global claim type. That might end up being more work than just spending a good amount of time on Types before I continue to do anything else. I'll think about it.

Main things to look at

src/renderer/component/wunderbar
src/renderer/redux/*/search
src/renderer/component/header
src/renderer/page/discover
src/renderer/component/link (still need to change link to button)
src/renderer/component/common/category-list (lots of changes to this)
src/renderer/component/fileCard
src/renderer/scss/gui.scss

What to ignore (for now)

Colors
Any src/renderer/page/* files that aren't src/page/discover I'll get to these next
src/renderer/scss/_vars.scss

@seanyesmunt seanyesmunt force-pushed the redesign-wip branch from 99b375c to e0002f2 Dec 14, 2017

@liamcardenas

This comment has been minimized.

Copy link
Contributor

liamcardenas commented Dec 14, 2017

Looking great!!

@seanyesmunt seanyesmunt force-pushed the redesign-wip branch 5 times, most recently from 613473d to a9fba6e Dec 18, 2017

@seanyesmunt seanyesmunt force-pushed the redesign-wip branch 4 times, most recently from 835641d to 46d4081 Dec 27, 2017

@seanyesmunt seanyesmunt changed the title [WIP] Redesign groundwork [WIP] Redesign groundwork, homepage, search Dec 28, 2017

@seanyesmunt seanyesmunt force-pushed the redesign-wip branch 6 times, most recently from 47a8525 to a771273 Jan 2, 2018

</div>
{obscureNsfw && this.state.hovered && <NsfwOverlay />}
{obscureNsfw && <NsfwOverlay />}

This comment has been minimized.

@seanyesmunt

seanyesmunt Jan 4, 2018

Author Member

still need to figure this out. will get it when we determine proper colors/how this will look. ignore for now

@@ -1,11 +1,12 @@
/* eslint-disable */

This comment has been minimized.

@seanyesmunt

seanyesmunt Jan 4, 2018

Author Member

Ignore this file

I'll come back to it on a different page

this._input.value = this.state.address; // this causes cursor to go to end of input
// if they choose the "search for {value}" in the suggestions
// it will contain the {query}?search
const choseDoSuggestedSearch = value.endsWith('?search');

This comment has been minimized.

@seanyesmunt

seanyesmunt Jan 4, 2018

Author Member

I'm not sure I like this, we need to choose to onSearch based on some flag for the Search for "{query}" suggestion

@@ -1,10 +1,12 @@
/* eslint-disable */

This comment has been minimized.

@seanyesmunt

seanyesmunt Jan 4, 2018

Author Member

ignore

@@ -1,3 +1,4 @@
/* eslint-disable */

This comment has been minimized.

@seanyesmunt

seanyesmunt Jan 4, 2018

Author Member

ignore

@@ -1,15 +1,21 @@
// @flow

This comment has been minimized.

@seanyesmunt

seanyesmunt Jan 4, 2018

Author Member

ignore

@@ -1,17 +1,24 @@
/* eslint-disable */

This comment has been minimized.

@seanyesmunt

seanyesmunt Jan 4, 2018

Author Member

ignore

src: url('../../../static/font/metropolis/Metropolis-SemiBold.woff2') format('woff2');
}

// @font-face {

This comment has been minimized.

@seanyesmunt

seanyesmunt Jan 4, 2018

Author Member

Still figuring out what font-weights we should use across the app.

Ignore for now

@@ -6,6 +6,8 @@ $width-page-constrained: 800px;
$text-color: #000;

This comment has been minimized.

@seanyesmunt

seanyesmunt Jan 4, 2018

Author Member

ignore the weird stuff going on here. still need to determine proper colors to use

@@ -2,321 +2,257 @@
margin-left: auto;

This comment has been minimized.

@seanyesmunt

seanyesmunt Jan 4, 2018

Author Member

ignore this for now, it will change a lot

I've commented out a lot to see what we actually need. Will be better suited for a review after I work on the file page

@seanyesmunt seanyesmunt changed the title [WIP] Redesign groundwork, homepage, search Redesign groundwork, homepage, search Jan 4, 2018

@seanyesmunt seanyesmunt requested review from kauffj and liamcardenas Jan 4, 2018

@kauffj
Copy link
Member

kauffj left a comment

Definitely a lot to work on but looks like a promising start! Some initial feedback.

constructor() {
super();
this.mainContent = undefined;
(this: any).scrollListener = this.scrollListener.bind(this);

This comment has been minimized.

@kauffj

kauffj Jan 4, 2018

Member

What is (this: any) doing here? I noticed this in several places.

This comment has been minimized.

@seanyesmunt

seanyesmunt Jan 5, 2018

Author Member

Without it there are flow issues. Got the fix from here facebook/flow#1517

@@ -1,25 +1,11 @@
// just disabling the linter because this file shouldn't even exist
// will continue to move components over to /components/common/{comp} - sean
/* eslint-disable */
import React from 'react';
import PropTypes from 'prop-types';
import { formatCredits, formatFullPrice } from 'util/formatCredits';
import lbry from '../lbry.js';

This comment has been minimized.

@kauffj

kauffj Jan 4, 2018

Member

Great call to refactor this file and eliminate this "common" bullshit :)

Any reason not to just make these normal / typical first-class components?

This comment has been minimized.

@seanyesmunt

seanyesmunt Jan 5, 2018

Author Member

I have been thinking of /common/* simple utility components. Then we can avoid the all of the connect(null, null)(Component) index.js files which a few components have.

const uris = [];
const actions = [];
dispatch({
type: ACTIONS.SEARCH_STARTED,

This comment has been minimized.

@kauffj

kauffj Jan 4, 2018

Member

Can you refactor these to the proper names while working on this?

This comment has been minimized.

@seanyesmunt

seanyesmunt Jan 5, 2018

Author Member

Yep, I'll add that

@@ -91,6 +91,10 @@ export const FILE_DELETE = 'FILE_DELETE';
export const SEARCH_STARTED = 'SEARCH_STARTED';
export const SEARCH_COMPLETED = 'SEARCH_COMPLETED';
export const SEARCH_CANCELLED = 'SEARCH_CANCELLED';
export const UPDATE_SEARCH_QUERY = 'UPDATE_SEARCH_QUERY';
export const GET_SEARCH_SUGGESTIONS_START = 'GET_SEARCH_SUGGESTIONS_START';

This comment has been minimized.

@kauffj

kauffj Jan 4, 2018

Member

IMO no need for GET_ in this name.

color: var(--color-primary);
.header__wunderbar {
flex: 1;
max-width: 325px;

This comment has been minimized.

@kauffj

kauffj Jan 4, 2018

Member

Have you tried this larger? Or just using all available space? Already, some results get cut-off, and this seems unnecessary when there is plenty of real estate.

This comment has been minimized.

@seanyesmunt

seanyesmunt Jan 5, 2018

Author Member

Yeah I think we will have it stretch to fill the space

font-weight: normal;
font-style: normal;
text-rendering: optimizeLegibility;
src: url('../../../static/font/metropolis/Metropolis-Medium.woff2') format('woff2');

This comment has been minimized.

@kauffj

kauffj Jan 4, 2018

Member

I think we are packaging some Metropolis font files we don't actually use. Intentional?

This comment has been minimized.

@seanyesmunt

seanyesmunt Jan 5, 2018

Author Member

I figured would just keep them all in for now so I didn't have to copy them in as I test them. We won't end using a lot of these.

html {
height: 100%;
font-size: var(--font-size);
// The actual fonts used will change ex: medium vs regular

This comment has been minimized.

@kauffj

kauffj Jan 4, 2018

Member

This started as the equivalent of an "everything" CSS file and likely contains some of the worst CSS in the project. Please do your best to refactor as you work on this.

This comment has been minimized.

@seanyesmunt

seanyesmunt Jan 5, 2018

Author Member

I plan to get rid of or move all of the specific styling. Then keep this is the global html/app-wide styling

@seanyesmunt seanyesmunt force-pushed the redesign-wip branch from 2e71b91 to c93f459 Jan 8, 2018

@seanyesmunt

This comment has been minimized.

Copy link
Member Author

seanyesmunt commented Jan 9, 2018

Just going to merge this since I have a few new commits locally. The next PR will be mostly touching the same files so any issues can be brought up on that.

@seanyesmunt seanyesmunt merged commit f92d48a into master Jan 9, 2018

2 checks passed

Linux (LBRY App) TeamCity build finished
Details
OSX (LBRY App) TeamCity build finished
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.