post detail page changes #869

alicewriteswrongs · 2018-07-03T15:46:07Z

What are the relevant tickets?

closes #834

What's this PR do?

This updates the detail page display of the post for the most recent designs. The design on the linked issue isn't the most up-to-date, look on invision instead.

Basically, some stuff is moved around, and some of the post actions are now moved into a little dropdown menu (similar to on the post list view). There is also a little 'share' menu, which lets you easily copy a link to the post or share it to your fb or whatever.

How should this be manually tested?

Check that the changes implement the design, and that the new stuff works alright. Nothing should be weird...

pdpinch · 2018-07-03T16:11:54Z

Do you have the link to Invision?

alicewriteswrongs · 2018-07-03T17:06:03Z

they're all in the 'detail' section, here: https://invis.io/B2MF7PWGMY7

alicewriteswrongs · 2018-07-03T17:06:45Z

this one in particular shows the final design for the buttons along the bottom: https://projects.invisionapp.com/share/B2MF7PWGMY7#/screens/305904717

codecov-io · 2018-07-03T17:07:37Z

Codecov Report

Merging #869 into master will decrease coverage by <.01%.
The diff coverage is 97.74%.

@@            Coverage Diff             @@
##           master     #869      +/-   ##
==========================================
- Coverage   96.31%   96.31%   -0.01%     
==========================================
  Files         321      323       +2     
  Lines       11124    11227     +103     
  Branches      374      374              
==========================================
+ Hits        10714    10813      +99     
- Misses        351      355       +4     
  Partials       59       59

Impacted Files	Coverage Δ
static/js/components/ProfileImageUploader.js	`84% <ø> (ø)`	⬆️
static/js/components/ExpandedPostDisplay_test.js	`100% <100%> (ø)`	⬆️
static/js/components/UserMenu.js	`100% <100%> (ø)`	⬆️
static/js/components/SharePopup.js	`100% <100%> (ø)`
static/js/lib/posts.js	`100% <100%> (ø)`	⬆️
static/js/components/CommentTree.js	`97.67% <100%> (+0.05%)`	⬆️
static/js/components/CompactPostDisplay.js	`100% <100%> (ø)`	⬆️
static/js/containers/ProfileImage_test.js	`100% <100%> (ø)`	⬆️
static/js/lib/url_test.js	`100% <100%> (ø)`	⬆️
static/js/containers/ProfileImage.js	`100% <100%> (ø)`	⬆️
... and 10 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 20d58aa...b84655d. Read the comment docs.

rhysyngsun · 2018-07-03T20:32:26Z

static/js/containers/ProfileImage.js

@@ -36,6 +36,7 @@ class ProfileImage extends React.Component<*> {
    startPhotoEdit: (p: File) => void,
    updatePhotoEdit: (b: Blob) => void,
    useSmall?: boolean,
+    useMicro?: boolean,


Rather than having multiple boolean fields for each type (which must/should all agree or we could end up in undefined behavior, e.g. <ProfileImage useMicro useSmall /> is valid with this implementation), I'd suggest passing a single size prop that is typed as a union and determine whether you're using micro, small, medium off of that.

yeah, that makes sense to me

unfortunately it seems that because of the connect call the props for the ProfileImage aren't actually checked anywhere when the component is mounted...seeing if there's an easy way to work around that

Would flow-typed defs for react-redux help flow infer that? https://github.com/flow-typed/flow-typed/blob/master/definitions/npm/react-redux_v5.x.x/flow_v0.63.0-/react-redux_v5.x.x.js

yeah I added them, unfortunately that broke flow typings for a bunch of other things so I had to fiddle with it for a while. I just got it all typechecking now (haven't pushed the commits yet though)

rhysyngsun · 2018-07-03T20:43:19Z

static/js/components/ExpandedPostDisplay.js

+              <ProfileImage
+                profile={{
+                  name:        post.author_name,
+                  image_small: post.profile_image


Here, because useMicro is specified makeProfileImageUrl ends up returning the default profile image because the useSmall value passed to it is undefined and there is no medium profile image passed here.

rhysyngsun · 2018-07-03T20:44:08Z

static/js/components/ExpandedPostDisplay.js

-              image_small: post.profile_image
-            }}
-            useSmall={true}
-          />
          <div className="post-title">{formatPostTitle(post)}</div>


I think the font size of this should increase to match the mockup. I'm uncertain as to what exact size it should be.

yeah, it's 28px on the Zeplin thing so I'll change it to that

rhysyngsun · 2018-07-03T20:45:11Z

static/js/components/ExpandedPostDisplay.js

-          >
-            <a href="#">report</a>
+        <div className="left">
+          <PostVotingButtons


Are you addressing the icon change for the upvote button in a separate PR?

The last I heard the design wasn't yet finalized, so I have held off on it so far

rhysyngsun · 2018-07-03T20:47:59Z

static/js/components/SharePopup.js

+        <input ref={ref => (this.input = ref)} readOnly value={url} />
+        <div className="bottom-row">
+          <div className="share-buttons">
+            <FacebookShareButton url={url}>


Nit: these buttons could probably use a little horizontal padding between each other

rhysyngsun · 2018-07-03T20:48:26Z

static/js/components/SharePopup.js

+      <div className="share-popup">
+        <div className="triangle" />
+        <div className="share-title">Share a link to this post:</div>
+        <input ref={ref => (this.input = ref)} readOnly value={url} />


This input hangs off the end of the popup:

Firefox seems to set a different default box-sizing on <input> tags than chrome, it should be fixed now

rhysyngsun · 2018-07-03T20:49:40Z

static/js/components/SharePopup.js

+    closePopup()
+  }
+
+  selectInputText = () => {


This needs some preventDefault() magic so the page doesn't scroll when you click it.

rhysyngsun · 2018-07-03T20:53:32Z

static/js/components/SharePopup.js

+import { FacebookIcon, TwitterIcon, LinkedinIcon } from "react-share"
+
+export class SharePopupHelper extends React.Component<*> {
+  input: ?HTMLInputElement


Should use React.createRef() instead: https://reactjs.org/docs/refs-and-the-dom.html#creating-refs

nice, I didn't know about the new API

rhysyngsun · 2018-07-03T20:56:53Z

static/js/components/ExpandedPostDisplay.js

+            </div>
+            {postShareMenuOpen ? (
+              <SharePopup
+                url={window.location.href}


I don't think we want to do window.location.href here because it's not always the canoncial link to the post. A counterexample can be seen if you click on the "permalink" button for a comment and then click the "share" button, you get the url to the comment, not the post, which I don't think is what we want.

makes sense, I'll write a library function for getting a post permalink

alicewriteswrongs · 2018-07-05T19:40:33Z

@rhysyngsun ok I think all your comments should be addressed now

alicewriteswrongs · 2018-07-05T19:47:11Z

nvm, I have one more thing to do (we don't want to show the social sharing buttons on private channels)

alicewriteswrongs · 2018-07-05T20:19:38Z

ok, I think things should be ready for review now

rhysyngsun

Looking good, just a few small things

rhysyngsun · 2018-07-05T21:23:54Z

static/js/containers/ProfileImage.js

-    const imageSizeClass = useSmall ? "small" : "medium"
+    const imageUrl = makeProfileImageUrl(
+      profile,
+      imageSize === "micro" || imageSize === "small"


Would it be a good idea to use constant values for these?

I was on the fence when I was writing it, on the one hand it would prevent typos, but Flow should do that automatically. One thing that is annoying about these union types is that it doesn't seem to allow you to declare constants (as JS variables) and then declare a type using those variables. I wanted to do something like this:

const PROFILE_IMAGE_MICRO = "micro" const PROFILE_IMAGE_SMALL = "small" const PROFILE_IMAGE_MEDIUM = "medium" type ImageSize = | PROFILE_IMAGE_MICRO | PROFILE_IMAGE_SMALL | PROFILE_IMAGE_MEDIUM

But when I tried it gave me an error about needing to use the typeof operator - but when I do that the type for ImageSize ends up being string | string | string! With the current setup for the ImageSize type it throws a type error if I pass, for instance, "Micro" as the imageSize prop, but if the type is string | string | string it will obviously accept any string.

I'm not sure if this is a limitation for Flow or I just don't know the right syntax. Anyway, so we could declare constants for the three strings, which would make it easier to change them in one place later if we wanted, but it doesn't seem like they can be used in the flow type :/

this provides a little context for that: facebook/flow#2639

I'll go ahead and declare constants to use when setting the size prop though

rhysyngsun · 2018-07-05T21:26:42Z

static/js/components/SharePopup_test.js

+
+  it("should hide buttons, if hideSocialButtons === true", () => {
+    const wrapper = renderSharePopupHelper({ hideSocialButtons: true })
+    ;[FacebookShareButton, LinkedinShareButton, TwitterShareButton].forEach(


Take it or leave it: one thing I've found useful that enzyme does is allow you to find components by their name (e.g. "FacebookShareButton", that has the benefit of not having to couple this test via an import and the test will still fail if the component changes and the test doesn't or vice versa.

so I gave that a try and, just changing all the component names to strings, my tests are failing. I think maybe the names of these components aren't set to the same thing as the same of the export (that can happen with things created with HOCs, and looking at the source the author wrote a system to create all the different icons with a HOC). I'm going to leave it as-is, since it looks like all the icons have the same name (CreatedButton).

ah ok, that's a bit odd that such a low-level component is an hoc

This updates the display of the post to match the most recent designs. This includes a refactor of how we display the little post action buttons, adds a new sharing menu, and a few other things. As part of this PR I also updated the flow-typed typedefs for all our installed dependencies. We were missing, somewhat crucially, the typedefs for react-redux, so none of our connected components were being properly type-checked (because Flow didn't know how the type signature of the `connect` function). So, now that that has been added, we should be in a more-completely-type-checked world. This change also involved a small refactor of the profile image component, to add a "micro" size option. closes #834 pr #869

alicewriteswrongs added the Needs Review label Jul 3, 2018

odlbot temporarily deployed to odl-open-discussions-ci-pr-869 July 3, 2018 15:46 Inactive

alicewriteswrongs temporarily deployed to odl-open-discussions-ci-pr-869 July 3, 2018 17:07 Inactive

rhysyngsun self-assigned this Jul 3, 2018

rhysyngsun requested changes Jul 3, 2018

View reviewed changes

rhysyngsun added Waiting on Author and removed Needs Review labels Jul 3, 2018

alicewriteswrongs force-pushed the ap/post-detail-change branch from eccf2a5 to 5ea94a9 Compare July 5, 2018 17:45

alicewriteswrongs temporarily deployed to odl-open-discussions-ci-pr-869 July 5, 2018 17:45 Inactive

alicewriteswrongs temporarily deployed to odl-open-discussions-ci-pr-869 July 5, 2018 18:06 Inactive

alicewriteswrongs force-pushed the ap/post-detail-change branch from 770dfb1 to a8c822e Compare July 5, 2018 19:19

alicewriteswrongs temporarily deployed to odl-open-discussions-ci-pr-869 July 5, 2018 19:19 Inactive

alicewriteswrongs temporarily deployed to odl-open-discussions-ci-pr-869 July 5, 2018 19:40 Inactive

alicewriteswrongs added Needs Review and removed Waiting on Author labels Jul 5, 2018

alicewriteswrongs force-pushed the ap/post-detail-change branch from 4b0f709 to df229f3 Compare July 5, 2018 20:13

alicewriteswrongs temporarily deployed to odl-open-discussions-ci-pr-869 July 5, 2018 20:14 Inactive

alicewriteswrongs force-pushed the ap/post-detail-change branch from df229f3 to 952041a Compare July 5, 2018 20:25

alicewriteswrongs temporarily deployed to odl-open-discussions-ci-pr-869 July 5, 2018 20:26 Inactive

alicewriteswrongs temporarily deployed to odl-open-discussions-ci-pr-869 July 5, 2018 20:53 Inactive

rhysyngsun requested changes Jul 5, 2018

View reviewed changes

rhysyngsun added Waiting on Author and removed Needs Review labels Jul 5, 2018

alicewriteswrongs force-pushed the ap/post-detail-change branch from 17c51cf to 2814eec Compare July 6, 2018 14:36

alicewriteswrongs temporarily deployed to odl-open-discussions-ci-pr-869 July 6, 2018 14:36 Inactive

rhysyngsun approved these changes Jul 6, 2018

View reviewed changes

alicewriteswrongs force-pushed the ap/post-detail-change branch from 2814eec to f048197 Compare July 6, 2018 16:00

alicewriteswrongs deployed to odl-open-discussions-ci-pr-869 July 6, 2018 16:00 View deployment

alicewriteswrongs force-pushed the ap/post-detail-change branch from f048197 to b84655d Compare July 6, 2018 16:16

alicewriteswrongs merged commit b84655d into master Jul 6, 2018

alicewriteswrongs requested a deployment to odl-open-discussions-ci-pr-869 July 6, 2018 16:16 Abandoned

alicewriteswrongs deployed to github-pages July 6, 2018 16:16 View deployment

alicewriteswrongs deleted the ap/post-detail-change branch July 6, 2018 19:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

post detail page changes #869

post detail page changes #869

alicewriteswrongs commented Jul 3, 2018

pdpinch commented Jul 3, 2018 via email

alicewriteswrongs commented Jul 3, 2018

alicewriteswrongs commented Jul 3, 2018

codecov-io commented Jul 3, 2018 •

edited

Loading

rhysyngsun Jul 3, 2018

alicewriteswrongs Jul 5, 2018

alicewriteswrongs Jul 5, 2018

rhysyngsun Jul 5, 2018

alicewriteswrongs Jul 5, 2018

rhysyngsun Jul 3, 2018

rhysyngsun Jul 3, 2018

alicewriteswrongs Jul 5, 2018

rhysyngsun Jul 3, 2018

alicewriteswrongs Jul 5, 2018

rhysyngsun Jul 3, 2018

rhysyngsun Jul 3, 2018

alicewriteswrongs Jul 5, 2018

rhysyngsun Jul 3, 2018

rhysyngsun Jul 3, 2018

alicewriteswrongs Jul 5, 2018

rhysyngsun Jul 3, 2018

alicewriteswrongs Jul 5, 2018

alicewriteswrongs commented Jul 5, 2018

alicewriteswrongs commented Jul 5, 2018

alicewriteswrongs commented Jul 5, 2018

rhysyngsun left a comment

rhysyngsun Jul 5, 2018

alicewriteswrongs Jul 6, 2018

alicewriteswrongs Jul 6, 2018

alicewriteswrongs Jul 6, 2018

rhysyngsun Jul 5, 2018

alicewriteswrongs Jul 6, 2018

alicewriteswrongs Jul 6, 2018

rhysyngsun Jul 6, 2018

post detail page changes #869

post detail page changes #869

Conversation

alicewriteswrongs commented Jul 3, 2018

What are the relevant tickets?

What's this PR do?

How should this be manually tested?

pdpinch commented Jul 3, 2018 via email

alicewriteswrongs commented Jul 3, 2018

alicewriteswrongs commented Jul 3, 2018

codecov-io commented Jul 3, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicewriteswrongs commented Jul 5, 2018

alicewriteswrongs commented Jul 5, 2018

alicewriteswrongs commented Jul 5, 2018

rhysyngsun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jul 3, 2018 •

edited

Loading