Add Status Bar to the desktop app #4259

2020-05-28T18:03:44+02:00

infinite-persistence commented

2020-05-28 18:03:44 +02:00

(Migrated from github.com)

PR Checklist

Please check all that apply to this PR using "x":

I have checked that this PR is not a duplicate of an existing PR (open, closed or merged)
I have checked that this PR does not introduce a breaking change
This PR introduces breaking changes and I have provided a detailed explanation below

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting)
Refactoring (no functional changes)
Documentation changes
Other - Please describe:

Fixes

Issue Number: #3776

What is the current behavior?

No status bar when hovering over a clickable link.

What is the new behavior?

Status bar (ala web browser) appears to show where a link is pointing to.

## PR Checklist  Please check all that apply to this PR using "x": - [x] I have checked that this PR is not a duplicate of an existing PR (open, closed or merged) - [x] I have checked that this PR does not introduce a breaking change - [ ] This PR introduces breaking changes and I have provided a detailed explanation below ## PR Type What kind of change does this PR introduce? - [ ] Bugfix - [x] Feature - [ ] Code style update (formatting) - [ ] Refactoring (no functional changes) - [ ] Documentation changes - [ ] Other - Please describe: ## Fixes Issue Number: #3776 ## What is the current behavior? No status bar when hovering over a clickable link. ## What is the new behavior? Status bar (ala web browser) appears to show where a link is pointing to.

neb-b (Migrated from github.com) reviewed 2020-05-28 18:03:44 +02:00

neb-b (Migrated from github.com) requested changes 2020-05-28 19:15:14 +02:00

ui/component/common/status-bar.jsx

					
				@ -0,0 +29,4 @@

				  }

				  handleUrlChange(event: any, url: string) {

				    // We want to retain the previous URL so that it can fade out

neb-b (Migrated from github.com) commented

2020-05-28 19:15:09 +02:00

The fade out animation is great 👍

ui/component/page/view.jsx

					
				@ -6,2 +6,4 @@

				import Header from 'component/header';

				import Footer from 'web/component/footer';

				/* @if TARGET='app' */

				import StatusBar from 'component/common/status-bar';

neb-b (Migrated from github.com) commented

2020-05-28 19:13:20 +02:00

This import statement should be wrapped in

// @if TARGET='web'

This import statement should be wrapped in ``` // @if TARGET='web' ```

web/component/footer.jsx

neb-b (Migrated from github.com) commented

2020-05-28 19:14:50 +02:00

Instead of adding a bunch of empty props. You can just add this below

let buttonProps = {};
if (link) {
  buttonProps.href = link;
}

if (navigate) {
  buttonProps.navigate = navigate;
}

return <li><Button ... {...buttonProps} ...

That should fix the flow errors

Instead of adding a bunch of empty props. You can just add this below ``` let buttonProps = {}; if (link) { buttonProps.href = link; } if (navigate) { buttonProps.navigate = navigate; } return <li><Button ... {...buttonProps} ... ``` That should fix the flow errors

neb-b commented

2020-05-28 19:15:44 +02:00

(Migrated from github.com)

This is awesome! I left a few comments, but it looks really good! And seems to work great. Can you also add a changelog addition?
https://github.com/lbryio/lbry-desktop/blob/master/CHANGELOG.md

This is awesome! I left a few comments, but it looks really good! And seems to work great. Can you also add a changelog addition? https://github.com/lbryio/lbry-desktop/blob/master/CHANGELOG.md

neb-b commented

2020-05-28 19:18:53 +02:00

(Migrated from github.com)

Another thing I just noticed. When I hover over the sidebar on the right. The link that shows in the status bar is lbry://$/following

It should probably show as /$/following

Another thing I just noticed. When I hover over the sidebar on the right. The link that shows in the status bar is `lbry://$/following` It should probably show as `/$/following`

infinite-persistence (Migrated from github.com) reviewed 2020-05-28 19:49:40 +02:00

web/component/footer.jsx

infinite-persistence (Migrated from github.com) commented

2020-05-28 19:49:40 +02:00

Something like this?

    <footer className="footer">
      {sections.map(({ name, links }) => {
        return (
          <div key={name} className="footer__section">
            <div className="footer__section-title">{name}</div>
            <ul className="ul--no-style">
              {links.map(({ label, link, navigate }) => {
                let buttonProps = {};
                if (link) {
                  buttonProps.href = link;
                }
                if (navigate) {
                  buttonProps.navigate = navigate;
                }

                return (
                  <li key={label}>
                    <Button className="footer__link" {...buttonProps} label={label} />
                  </li>
                );
              })}
            </ul>
          </div>
        );
      })}
    </footer>

But flow seems still unhappy:

Something like this? ``` <footer className="footer"> {sections.map(({ name, links }) => { return ( <div key={name} className="footer__section"> <div className="footer__section-title">{name}</div> <ul className="ul--no-style"> {links.map(({ label, link, navigate }) => { let buttonProps = {}; if (link) { buttonProps.href = link; } if (navigate) { buttonProps.navigate = navigate; } return ( <li key={label}> <Button className="footer__link" {...buttonProps} label={label} /> </li> ); })} </ul> </div> ); })} </footer> ``` But flow seems still unhappy: ![image](https://user-images.githubusercontent.com/64950861/83175542-a0789700-a14e-11ea-9115-f35c16bf16a4.png)

neb-b (Migrated from github.com) reviewed 2020-05-28 21:32:45 +02:00

web/component/footer.jsx

neb-b (Migrated from github.com) commented

2020-05-28 21:32:45 +02:00

Ugh. I guess you can just add a flow comment above that line
// $FlowFixMe

That should "fix" it.

Ugh. I guess you can just add a flow comment above that line `// $FlowFixMe` That should "fix" it.

infinite-persistence commented

2020-05-29 08:26:22 +02:00

(Migrated from github.com)

Summary of fixes for the re-commits above ^

Fix lbry:// protocol mistakenly shown for non-claims.
Disabled the flow issue instead of adding empty props.
Only include the status-bar file for 'app'.
Added changelog

#### Summary of fixes for the re-commits above ^ - [x] Fix `lbry://` protocol mistakenly shown for non-claims. - [x] Disabled the flow issue instead of adding empty props. - [x] Only include the status-bar file for 'app'. - [x] Added changelog

infinite-persistence commented

2020-05-29 08:52:58 +02:00

(Migrated from github.com)

I squashed my new changes into the old commits. That will make it cleaner for the final merge, but I realized it will be hard for you to see what has recently change. Hopefully this is ok. Let me know if you have any preferences about this for future PRs.

kauffj commented

2020-06-01 21:06:54 +02:00

(Migrated from github.com)

great idea @infinite-persistence!

neb-b (Migrated from github.com) requested changes 2020-06-02 22:18:58 +02:00

neb-b (Migrated from github.com) left a comment

One last comment then this is good to go!

electron/createWindow.js

					
				@ -167,0 +172,4 @@

				      dispUrl = 'Home';

				    } else if (dispUrl.startsWith(lbryProto + '$/')) {

				      dispUrl = dispUrl.replace(lbryProto, '/');

				    }

neb-b (Migrated from github.com) commented

2020-06-02 22:18:44 +02:00

I think this also needs a line that just returns "Home"

Right now I see "lbry://"

I think this also needs a line that just returns "Home" Right now I see "lbry://"

infinite-persistence commented

2020-06-03 05:49:15 +02:00

(Migrated from github.com)

Summary of fixes for the re-commits

Updated the Home's hover URL to show "Home" instead of "lbry://".
- I was going back and forth between "/" and "lbry://", but "Home" is way better.

--- a/electron/createWindow.js
+++ b/electron/createWindow.js
@@ -167,8 +167,10 @@ export default appState => {
   window.webContents.on('update-target-url', (event, url) => {
     // Change internal links to the lbry protocol. External (https) links should remain unchanged.
     let dispUrl = url.replace(`http://localhost:${WEBPACK_ELECTRON_PORT}/`, lbryProto);
-    if (dispUrl.startsWith(lbryProto + '$/')) {
-      // Non-claims don't need the lbry protocol.
+    // Non-claims don't need the lbry protocol:
+    if (dispUrl === lbryProto)  {
+      dispUrl = 'Home';
+    } else if (dispUrl.startsWith(lbryProto + '$/')) {
       dispUrl = dispUrl.replace(lbryProto, '/');
     }
     window.webContents.send('update-target-url', dispUrl);

#### Summary of fixes for the re-commits - [x] Updated the Home's hover URL to show "Home" instead of "lbry://". - I was going back and forth between "/" and "lbry://", but "Home" is way better. ``` --- a/electron/createWindow.js +++ b/electron/createWindow.js @@ -167,8 +167,10 @@ export default appState => { window.webContents.on('update-target-url', (event, url) => { // Change internal links to the lbry protocol. External (https) links should remain unchanged. let dispUrl = url.replace(`http://localhost:${WEBPACK_ELECTRON_PORT}/`, lbryProto); - if (dispUrl.startsWith(lbryProto + '$/')) { - // Non-claims don't need the lbry protocol. + // Non-claims don't need the lbry protocol: + if (dispUrl === lbryProto) { + dispUrl = 'Home'; + } else if (dispUrl.startsWith(lbryProto + '$/')) { dispUrl = dispUrl.replace(lbryProto, '/'); } window.webContents.send('update-target-url', dispUrl); ```

Sign in to join this conversation.

No reviewers

neb-b