PR#589: make whole feed item a link - fedora-hubs

fedora-hubs

#589 make whole feed item a link

Merged 6 years ago by abompard. Opened 6 years ago by ryanlerch.

ryanlerch/fedora-hubs feed-links into develop

make whole feed item a link

Ryan Lerch • 6 years ago

fd4c710

js/app/components/feed/Feed.js

file modified

+1 -1

		`@@ -26,7 +26,7 @@`
		`);`
		`});`
		`return (`
		`- <div className="Feed">`
		`+ <div className="Feed list-group list-group-flush">`
		`{ (items.length === 0 && this.props.loaded) ?`
		`<p className="m-2">`
		`<FormattedMessage {...messages.no_items} tagName="i" />`

js/app/components/feed/Icon.js

file modified

+3 -4

		`@@ -3,16 +3,15 @@`
		`export default class Icon extends React.Component {`
		`render() {`
		`return (`
		`- <a href={this.props.item.link ? this.props.item.link : '#'}`
		`- target="_blank"`
		`- className="d-flex align-self-start mr-2"`
		`+ <div`
		`+ className="align-self-start mr-2"`
		`>`
		`<img`
		`alt="User avatar"`
		`className="square-32 img-circle"`
		`src={this.props.item.secondary_icon}`
		`/>`
		`- </a>`
		`+ </div>`
		`);`
		`}`
		`}`

js/app/components/feed/Panel.js

file modified

+19 -3

		`@@ -22,8 +22,24 @@`
		`}`

		`render() {`
		`+ let Element, props;`
		`+ if (this.props.item.link) {`
		`+ Element = "a";`
		`+ props = {`
		`+ className: "list-group-item list-group-item-action d-flex",`
		`+ href: this.props.item.link,`
		`+ target: "_blank",`
		`+ };`
		`+ } else {`
		`+ Element = "div";`
		`+ props = {`
		`+ className: "list-group-item d-flex",`
		`+ };`
		`+ }`
		`return (`
		`- <div className="media py-3 px-2" id={this.props.item.id}>`
		`+ <Element {...props}`
		`+ id={this.props.item.id}`
		`+ >`
		`<Icon item={this.props.item} />`
		`<Markup`
		`{...this.props}`
		`@@ -37,12 +53,12 @@`
		`Object.keys(this.props.item.msg_ids).length > 1) &&`
		`<div className="mt-auto">`
		`<small>`
		`- <button className="btn btn-link" onClick={this.toggleDetails}>details</button>`
		`+ <button className="btn btn-link p-0" onClick={this.toggleDetails}>details</button>`
		`</small>`
		`</div>`
		`}`
		`</div>`
		`- </div>`
		`+ </Element>`
		`);`
		`}`

ryanlerch commented 6 years ago

this makes the feeds use the bootstrap list-groups,
and makes the whole feed item a link to the thing that
relates to the feed item. Other links will still work inside
the item, like username urls and buttons.

fixes #562

abompard commented on line 9 of js/app/components/feed/Panel.js 6 years ago

Should we do something better when we have no link property? This will scroll the page back to the top and add a hash... I see two options:
- use a div and not a a when there's no link property
- use a onClick handler like: onClick={(e) => {e.preventDefault()}}.

The second one may be easier.

abompard commented 6 years ago

This seems to work:

--- a/js/app/components/feed/Panel.js
+++ b/js/app/components/feed/Panel.js
@@ -12,6 +12,7 @@ export default class Panel extends React.Component {
       detailsOpened: false,
     }
     this.toggleDetails = this.toggleDetails.bind(this);
+    this.handleClick = this.handleClick.bind(this);
   }

   toggleDetails(e) {
@@ -21,6 +22,12 @@ export default class Panel extends React.Component {
     ));
   }

+  handleClick(e) {
+    if (!this.props.item.link) {
+      e.preventDefault();
+    }
+  }
+
   render() {
     return (
       <a 
@@ -28,6 +35,7 @@ export default class Panel extends React.Component {
         id={this.props.item.id}
         href={this.props.item.link ? this.props.item.link : '#'}
         target="_blank"
+        onClick={this.handleClick}
         >
         <Icon item={this.props.item} />
         <Markup
@@ -42,7 +50,7 @@ export default class Panel extends React.Component {
              Object.keys(this.props.item.msg_ids).length > 1) &&
             <div className="mt-auto">
               <small>
-                <button className="btn btn-link" onClick={this.toggleDetails}>details</button>
+                <button className="btn btn-link p-0" onClick={this.toggleDetails}>details</button>
               </small>
             </div>
           }

I also aligned the "details" link with the "save" button, it looked weird.

abompard commented 6 years ago

And this seems to work too. It's a bit prettier because it does not generate a tags, so the cursor does not turn into a pointer on hover.

--- a/js/app/components/feed/Panel.js
+++ b/js/app/components/feed/Panel.js
@@ -22,12 +22,23 @@ export default class Panel extends React.Component {
   }

   render() {
+    let Element, props;
+    if (this.props.item.link) {
+      Element = "a";
+      props = {
+        className: "list-group-item list-group-item-action d-flex",
+        href: this.props.item.link,
+        target: "_blank",
+      };
+    } else {
+      Element = "div";
+      props = {
+        className: "list-group-item d-flex",
+      };
+    }
     return (
-      <a 
-        className="list-group-item list-group-item-action d-flex" 
+      <Element {...props}
         id={this.props.item.id}
-        href={this.props.item.link ? this.props.item.link : '#'}
-        target="_blank"
         >
         <Icon item={this.props.item} />
         <Markup
@@ -42,12 +53,12 @@ export default class Panel extends React.Component {
              Object.keys(this.props.item.msg_ids).length > 1) &&
             <div className="mt-auto">
               <small>
-                <button className="btn btn-link" onClick={this.toggleDetails}>details</button>
+                <button className="btn btn-link p-0" onClick={this.toggleDetails}>details</button>
               </small>
             </div>
           }
         </div>
-      </a>
+      </Element>
     );
   }

Your choice! :-)

ryanlerch commented 6 years ago

Thanks @abompard

yeah, i think the second option is the wayto go! i tried to do this, but couldnt figure it out! didnt know about that Element trick!

thanks! applied the second patch, and rebased.

cheers,
ryanlerch

rebased onto 0a7dfc69b207edb06265a786b9727f265c6e3b4f

6 years ago

rebased onto fd4c710

6 years ago