Font, color and margins style for Discussions UI #95

roberthouse54 · 2017-08-04T20:53:27Z

What are the relevant tickets?

This is the First round of style changes for the discussions. I am going to submit small incremental PRs as I go along with this.

What's this PR do?

Sets some of the basic colors, fonts, etc for discussions.

What Issue is this related to

#85

alicewriteswrongs

just a few comments on the React changes

alicewriteswrongs · 2017-08-07T14:37:10Z

static/js/components/ChannelBreadcrumbs.js

      <Link to="/">Discussions</Link>&nbsp;
      <span>&gt;</span>&nbsp;
-      <Link to={`/channel/${channel.name}`}>{channel.title}</Link>
+      {channel.title}


This should probably remain a <Link /> - we show the ChannelBreadcrumbs component on the page for each post, so it supplies a link back to the subreddit the post is from.

OK, that is fine.

alicewriteswrongs · 2017-08-07T14:39:09Z

static/js/containers/ChannelPage.js

@@ -55,6 +56,7 @@ class ChannelPage extends React.Component {
              <ChannelSidebar channel={channel} />
            </Card>
          </div>
+          <br className="clear"/>


is there any way we can avoid using <br />? I just find it yucky, haha

I am open to any solutions you can come up with, although this is in my opinion, a basic failure with the CSS model and contained floats inside divs. But I am open to any way you'd prefer to do this. I do want those divs actually contained inside the container div though.

Maybe I just don't understand what it's doing, but if I delete this element I don't see any changes in the display on the page. I'll leave it in for now if it's necessary.

roberthouse54 · 2017-08-07T16:06:29Z

I will be out today and tomorrow. @aliceriot , can you make the required changes and merge the PR?

roberthouse54 · 2017-08-07T16:14:18Z

One further comment:

The channel page UI, and the Home page both use a "Post Summary" component, that displays the Post title, post author, number of comments, and upvoting. The Post Details page is currently reusing this same component. But we should really create a different component for the Post Details page as the design is really not the same as it is on the channel/home page.

alicewriteswrongs · 2017-08-07T18:21:05Z

On the PostSummary component, it basically has two modes. On a post detail view it shows more information (like the text if its a text post), displays slightly differently, etc. Do you think that we could just continue to build out the two different display modes? If we want to style them differently we could have a css class which only appears on the 'expanded' view, for instance. There is a fair amount of display which is more or less identical looking in the designs (i.e. we show the link title, the author, etc) which is why I re-used the component for the 'channel' view.

alicewriteswrongs · 2017-08-07T18:30:15Z

the post display is a little messed up:

I'm going to make a few quick changes to restore the spacing

codecov-io · 2017-08-07T18:31:26Z

Codecov Report

Merging #95 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #95      +/-   ##
==========================================
+ Coverage   95.52%   95.53%   +0.01%     
==========================================
  Files          89       89              
  Lines        2033     2038       +5     
  Branches       63       63              
==========================================
+ Hits         1942     1947       +5     
  Misses         82       82              
  Partials        9        9

Impacted Files	Coverage Δ
static/js/components/ChannelSidebar.js	`100% <ø> (ø)`	⬆️
static/js/components/PostDisplay.js	`100% <ø> (ø)`	⬆️
static/js/containers/ChannelPage.js	`100% <ø> (ø)`	⬆️
static/js/components/ChannelBreadcrumbs.js	`100% <ø> (ø)`	⬆️
static/js/containers/HomePage.js	`100% <ø> (ø)`	⬆️
static/js/components/Card.js	`100% <100%> (ø)`	⬆️
static/js/components/Card_test.js	`100% <100%> (ø)`	⬆️
static/js/components/PostDisplay_test.js	`100% <100%> (ø)`	⬆️

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 68ecf6e...e4130ad. Read the comment docs.

roberthouse54 · 2017-08-07T19:19:25Z

The post display component is identical on the channel page and the home page, and yse I did see that there is an "expanded" version. But I think the layout on the actual post page is significantly different enough to warrant a different component and different html. The # of comments is down below, next to the comments UI, the upvoting is in a different location, etc

roberthouse54 · 2017-08-07T19:21:15Z

About using the break with class="clear"... yes it is a workaround. And I think you might not see any visual changes on the page. But the two floated divs are not actually contained inside the container div. I do want to have that working,as I think, otherwise, we will not be able to apply box property rules to the container etc.

alicewriteswrongs · 2017-08-07T19:23:43Z

it seems like setting display: flex on the container div (.double-column or .single-column) has the same effect, I think?

alicewriteswrongs · 2017-08-07T19:38:39Z

I'll leave it as-is for now and we can discuss when you're in

rhysyngsun

look good functionally, but noted one thing:

rhysyngsun · 2017-08-07T20:03:31Z

static/js/components/PostDisplay.js

@@ -46,19 +44,23 @@ export default class PostDisplay extends React.Component {
    return (
      <div className="post-summary">
        <div className="upvotes">
-          <button> &uArr;</button>
+          <img className="upvote-arrow" src="/static/images/upvote_arrow.png" />


Is there a reason for the move away from a <button>? This has accessibility issues and has no native disabled behavior (which is something I'm doing in #93, which will merge soon too).

let's defer to what you have in #93 for now

rhysyngsun

👍 when tests pass

This adjusts styles on a few things, mainly tweaking colors and adjusting layout / margins. It also: - adds a title prop to `<Card />`, which will add a consistently formatted title - does some CSS cleanup / reorganization

This just removes a line which was deleted in #95 and reincluded in #90 pr #103

alicewriteswrongs self-assigned this Aug 7, 2017

alicewriteswrongs suggested changes Aug 7, 2017

View reviewed changes

roberthouse54 mentioned this pull request Aug 7, 2017

CSS Styling #85

Closed

6 tasks

alicewriteswrongs force-pushed the rh-discussions-styles branch from 8f9363e to a2399c3 Compare August 7, 2017 18:31

alicewriteswrongs added the Needs Review label Aug 7, 2017

alicewriteswrongs removed their assignment Aug 7, 2017

rhysyngsun self-assigned this Aug 7, 2017

rhysyngsun requested changes Aug 7, 2017

View reviewed changes

alicewriteswrongs force-pushed the rh-discussions-styles branch from b23e5b0 to ef71a7a Compare August 7, 2017 20:08

rhysyngsun approved these changes Aug 7, 2017

View reviewed changes

rhysyngsun added Waiting on Author and removed Needs Review labels Aug 7, 2017

Made first round of css changes

e4130ad

This adjusts styles on a few things, mainly tweaking colors and adjusting layout / margins. It also: - adds a title prop to `<Card />`, which will add a consistently formatted title - does some CSS cleanup / reorganization

alicewriteswrongs force-pushed the rh-discussions-styles branch from ef71a7a to e4130ad Compare August 7, 2017 20:32

alicewriteswrongs merged commit e4130ad into master Aug 7, 2017

alicewriteswrongs deleted the rh-discussions-styles branch August 7, 2017 20:32

rhysyngsun mentioned this pull request Aug 8, 2017

Integrate UI for upvoting posts #93

Merged

alicewriteswrongs mentioned this pull request Aug 9, 2017

remove accidental change #103

Merged

alicewriteswrongs added a commit that referenced this pull request Aug 9, 2017

Remove unintentionally re-included line

e3a0377

This just removes a line which was deleted in #95 and reincluded in #90 pr #103

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Font, color and margins style for Discussions UI #95

Font, color and margins style for Discussions UI #95

roberthouse54 commented Aug 4, 2017 •

edited

alicewriteswrongs left a comment

alicewriteswrongs Aug 7, 2017

roberthouse54 Aug 7, 2017

alicewriteswrongs Aug 7, 2017

roberthouse54 Aug 7, 2017

alicewriteswrongs Aug 7, 2017

roberthouse54 commented Aug 7, 2017

roberthouse54 commented Aug 7, 2017

alicewriteswrongs commented Aug 7, 2017

alicewriteswrongs commented Aug 7, 2017

codecov-io commented Aug 7, 2017 •

edited

roberthouse54 commented Aug 7, 2017

roberthouse54 commented Aug 7, 2017

alicewriteswrongs commented Aug 7, 2017

alicewriteswrongs commented Aug 7, 2017

rhysyngsun left a comment

rhysyngsun Aug 7, 2017

alicewriteswrongs Aug 7, 2017

rhysyngsun left a comment

Font, color and margins style for Discussions UI #95

Font, color and margins style for Discussions UI #95

Conversation

roberthouse54 commented Aug 4, 2017 • edited

What are the relevant tickets?

What's this PR do?

What Issue is this related to

alicewriteswrongs left a comment

Choose a reason for hiding this comment

alicewriteswrongs Aug 7, 2017

Choose a reason for hiding this comment

roberthouse54 Aug 7, 2017

Choose a reason for hiding this comment

alicewriteswrongs Aug 7, 2017

Choose a reason for hiding this comment

roberthouse54 Aug 7, 2017

Choose a reason for hiding this comment

alicewriteswrongs Aug 7, 2017

Choose a reason for hiding this comment

roberthouse54 commented Aug 7, 2017

roberthouse54 commented Aug 7, 2017

alicewriteswrongs commented Aug 7, 2017

alicewriteswrongs commented Aug 7, 2017

codecov-io commented Aug 7, 2017 • edited

Codecov Report

roberthouse54 commented Aug 7, 2017

roberthouse54 commented Aug 7, 2017

alicewriteswrongs commented Aug 7, 2017

alicewriteswrongs commented Aug 7, 2017

rhysyngsun left a comment

Choose a reason for hiding this comment

rhysyngsun Aug 7, 2017

Choose a reason for hiding this comment

alicewriteswrongs Aug 7, 2017

Choose a reason for hiding this comment

rhysyngsun left a comment

Choose a reason for hiding this comment

roberthouse54 commented Aug 4, 2017 •

edited

codecov-io commented Aug 7, 2017 •

edited