Fixes #5043 - Add download button to non-owner shot page

[punamdahiya]

No description provided.

[jaredhirsch]

@punamdahiya Could you add a description of what this PR is supposed to do?

[punamdahiya]

@punamdahiya Could you add a description of what this PR is supposed to do?

@6a68 PR is bringing back download button on !owned shot page as shown here #5043 (comment)

[punamdahiya]

Moved downloadButton outside of ownership check, as want to display it for both owned and !owned shots

[punamdahiya]

For !owned shots, we want download button to show on the same row as shot title and don't need shot-alt-actions class that renders alt action buttons in separate row on medium and small window widths

[jaredhirsch]

Breaking the header into a second line at small widths seems kind of important, but here it's only encoded in a bit of conditional CSS. I think it'd be better to be explicit in the React component about which items are which. How about adding a shotActions prop to the ShotPageHeader that takes any buttons we want to show in the owner-only palette? Then the children will always be rendered on the top line.

[punamdahiya]

@6a68 I am confused by suggested shotActions prop on ShotPageHeader, is it a boolean that sets if we want shotactions button (favorite, edit, copy, delete, settings) shown or not.
Our buttons are defined and logic to show/hide is in view.js and gets rendered inside ShotPageHeader as props.children

[punamdahiya]

@6a68 Taken a stab at keeping logic of owner only buttons and its styles together in view.js. My understanding to bring logic of owner only palette inside ShotPageHeader component, we need to bring in defining all buttons and its related methods inside the component. Let me know if I am missing a better approach here. Thanks

[jaredhirsch]

@6a68 I am confused by suggested shotActions prop on ShotPageHeader, is it a boolean that sets if we want shotactions button (favorite, edit, copy, delete, settings) shown or not.
Our buttons are defined and logic to show/hide is in view.js and gets rendered inside ShotPageHeader as props.children

Sorry, missed your question. We can keep the logic in view.js, and just have two different ways to pass rendered buttons into the ShotPageHeader: the children prop and another prop. I'll write a little code to explain what I had in mind

[jaredhirsch]

@punamdahiya Here's a second attempt at an explanation:

Instead of passing all the elements into the ShotPageHeader using the special children prop, have two props: one for buttons that behave differently at narrow widths (call it shotActions, maybe), and keep using the children prop for the buttons that don't wrap around (either the download button, or the # button).

Elaborating a little bit in code:

• Inside view.js, put the buttons for the owner-only actions in an array:

const shotActions = this.props.isOwner ?
  [favoriteShotButton, editButton, copyButton, downloadButton, trashOrFlagButton] : null

• Also inside view.js, pass shotActions into a new shotActions prop on the ShotPageHeader, and pass either the download button or the signin button as the children:

render() {
  return (
  <ShotPageHeader ... shotActions={shotActions}>
    {this.props.isOwner ? downloadButton : signInButton}
  </ShotPageHeader>
);

• Add the new shotActions prop to ShotPageHeader, and construct the wrapper div inside a helper method:

  // inside ShotPageHeader.js 
  renderShotActions() {
    return (
      this.props.shotActions ?
        <div class="shot-alt-actions">{this.props.shotActions}</div> : null
    )
  }

• Inside the render method for ShotPageHeader, insert the shotActions in the appropriate spot, just before the children:

  // inside the ShotPageHeader render method:
  const shotActions = this.renderShotActions();
  return (
       <Header shouldGetFirefox={this.props.shouldGetFirefox} isOwner={this.props.isOwner}
              hasFxa={this.props.isFxaAuthenticated}>
        { myShotsText }
        { shotInfo }
        { shotActions }
        { this.props.children }
      </Header>

This will give us a clear separation between elements that are rendered as a separate menu of buttons (the shotActions) and elements that are always rendered in the header (download / signin button).

I hope this clarifies what I was asking for earlier, but if it's still unclear, let me know :-)

[punamdahiya]

Keeping signin here as signin button is defined in shot page header component

[punamdahiya]

This check isn't elegant but keeping it in favor of executing a block of code that's not needed for !owned shots

[punamdahiya]

@6a68 Updated PR with the suggested approach, thanks for detailed feedback!

[jaredhirsch]

React has some complaints:

Looks like the PropTypes need to be updated, and you'll need to set a key attribute on the buttons. Probably the simplest thing is just to set the key to the button-specific class when the buttons are created, like <div className="favorite-shot-button" key="favorite-shot-button">. They keys just need to be unique among siblings in a list, but globally-unique identifiers work, too.

See the keys doc and the more in-depth keys doc if you want to learn more about the details. I think keys as a hint to the virtual DOM algorithm matters a lot more when you're, say, trying to minimize DOM operations when updating a Facebook home feed. In our case, it's more of an annoyance / curiosity, but we might as well follow the framework's advice.

[punamdahiya]

Thanks for catching console warnings! Updated PropTypes and key attributes in PR.

[punamdahiya]

Thanks for catching console warnings! Updated PropTypes and key attributes in PR.

keys docs shared above is informative. Good to know that key don't need to be unique globally. Updating with more meaningful key values

[jaredhirsch]

LGTM! Nice work