LBRYCommunity/lbry-desktop

Fork

You've already forked lbry-desktop

Code Issues 596 Pull requests 6 Projects Releases 238 Packages Wiki Activity

Stream Key Button #7127

Merged

GlobalGamer2015 merged 12 commits from master into master

2021-09-23 08:17:59 +02:00

Conversation 2 Commits 12 Files changed 4 +104 -2

GlobalGamer2015 commented

2021-09-19 02:10:09 +02:00

(Migrated from github.com)

Copy link

Fixes

Stream Key is now hidden instead of showing.
Adds Stream Key Button to show and hide Stream Key.

Issue Number: 6494

What is the current behavior?

Stream Key is visible

What is the new behavior?

Stream Key is now hidden
Ability to show and hide Stream Key

What kind of change does this PR introduce?

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

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 added a line describing my change to CHANGELOG.md
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

## Fixes Stream Key is now hidden instead of showing. Adds Stream Key Button to show and hide Stream Key. Issue Number: 6494 ## What is the current behavior? Stream Key is visible ## What is the new behavior? Stream Key is now hidden Ability to show and hide Stream Key What kind of change does this PR introduce? - [x] Bugfix - [x] Feature - [ ] Code style update (formatting) - [ ] Refactoring (no functional changes) - [ ] Documentation changes - [ ] Other - Please describe: 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 added a line describing my change to CHANGELOG.md - [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 </details>

🎉 1

tzarebczan commented

2021-09-20 16:20:06 +02:00

(Migrated from github.com)

Copy link

Hey @GlobalGamer2015, nice to see you checking out the Odysee repo! Thanks for the PR! We'll get a review on this soon.

👍 1

infinite-persistence (Migrated from github.com) requested changes 2021-09-21 08:44:21 +02:00

CHANGELOG.md

infinite-persistence (Migrated from github.com) commented

2021-09-21 08:12:18 +02:00

Copy link

Please remove this, and add your entry at the top (under "[Unreleased for Desktop] > Added"), using the same format -as the other entries.
Run the lint commands. It does not pass the lint checker.

- [ ] Please remove this, and add your entry at the top (under "[Unreleased for Desktop] > Added"), using the same format -as the other entries. - [ ] Run the lint commands. It does not pass the lint checker.

ui/component/copyableStreamkey/view.jsx

infinite-persistence (Migrated from github.com) commented

2021-09-21 08:30:44 +02:00

Copy link

Rename to actual function name for clarity

ui/component/copyableStreamkeyUnmask/view.jsx

infinite-persistence (Migrated from github.com) commented

2021-09-21 08:29:26 +02:00

Copy link

It's unnecessary to create a separate Component just for unmask functionality, and it is also not the React-way to tweak the DOM directly.

The unmasking button can be encapsulated within the new copyableStreamkey itself.
Other alternatives:
- Skip copyableStreamkey, then augment the original copyableText to support an optional parameter isPassword (or enableMask or something).
- Or just do the masking logic in the livestream component, just like how we hid the access token in the Help Page.

It's unnecessary to create a separate Component just for unmask functionality, and it is also not the React-way to tweak the DOM directly. - The unmasking button can be encapsulated within the new `copyableStreamkey` itself. - Other alternatives: - Skip `copyableStreamkey`, then augment the original `copyableText` to support an optional parameter `isPassword` (or `enableMask` or something). - Or just do the masking logic in the livestream component, just like how we hid the access token in the Help Page.

infinite-persistence (Migrated from github.com) commented

2021-09-21 08:38:15 +02:00

Copy link

As mentioned in a prior comment, a separate component is not necessary.
On top of that, the button location is not ideal, and there is no gap between components. Here's one suggestion:

As mentioned in a prior comment, a separate component is not necessary. On top of that, the button location is not ideal, and there is no gap between components. Here's one suggestion: ![Untitled](https://user-images.githubusercontent.com/64950861/134123099-7795a82f-63ad-4050-bb7a-75776b794681.png)

infinite-persistence (Migrated from github.com) commented

2021-09-21 08:43:47 +02:00

Copy link

It is not necessary to show a toast when hiding and showing the key.

GlobalGamer2015 (Migrated from github.com) reviewed 2021-09-22 06:11:18 +02:00

CHANGELOG.md

GlobalGamer2015 (Migrated from github.com) commented

2021-09-22 06:11:18 +02:00

Copy link

Done, it passed lint checker on my end.

GlobalGamer2015 (Migrated from github.com) reviewed 2021-09-22 06:12:08 +02:00

ui/component/copyableStreamkeyUnmask/view.jsx

GlobalGamer2015 (Migrated from github.com) commented

2021-09-22 06:12:08 +02:00

Copy link

I removed the Component entirely.

GlobalGamer2015 (Migrated from github.com) reviewed 2021-09-22 06:13:42 +02:00

ui/component/copyableStreamkeyUnmask/view.jsx

GlobalGamer2015 (Migrated from github.com) commented

2021-09-22 06:13:42 +02:00

Copy link

I'm not that great with React so I'm not able to replicate that.

GlobalGamer2015 (Migrated from github.com) reviewed 2021-09-22 06:14:12 +02:00

ui/component/copyableStreamkeyUnmask/view.jsx

GlobalGamer2015 (Migrated from github.com) commented

2021-09-22 06:14:12 +02:00

Copy link

Removed this Component entirely.

infinite-persistence (Migrated from github.com) requested changes 2021-09-22 07:22:01 +02:00

infinite-persistence (Migrated from github.com) left a comment

Copy link

Thanks for the update!

CHANGELOG.md

infinite-persistence (Migrated from github.com) commented

2021-09-22 07:08:00 +02:00

Copy link

Please undo this. You'll just need to add Stream Key is now hidden. ([#7127](https://github.com/lbryio/lbry-desktop/pull/7127)) into the existing ### Fixed entry under ## [Unreleased for Desktop]. No need to bump the version.

Please undo this. You'll just need to add `Stream Key is now hidden. ([#7127](https://github.com/lbryio/lbry-desktop/pull/7127))` into the existing `### Fixed` entry under `## [Unreleased for Desktop]`. No need to bump the version.

package.json

infinite-persistence (Migrated from github.com) commented

2021-09-22 07:06:22 +02:00

Copy link

Please undo this.

ui/component/copyableStreamkey/index.js

infinite-persistence (Migrated from github.com) commented

2021-09-22 07:09:43 +02:00

Copy link

Rename to CopyableStreamkey

Rename to `CopyableStreamkey`

GlobalGamer2015 (Migrated from github.com) reviewed 2021-09-23 04:46:06 +02:00

package.json

GlobalGamer2015 (Migrated from github.com) commented

2021-09-23 04:46:06 +02:00

Copy link

Undone.

GlobalGamer2015 (Migrated from github.com) reviewed 2021-09-23 04:46:21 +02:00

CHANGELOG.md

GlobalGamer2015 (Migrated from github.com) commented

2021-09-23 04:46:21 +02:00

Copy link

Undone and placed in correct area.

GlobalGamer2015 (Migrated from github.com) reviewed 2021-09-23 04:46:34 +02:00

ui/component/copyableStreamkey/index.js

GlobalGamer2015 (Migrated from github.com) commented

2021-09-23 04:46:34 +02:00

Copy link

Renamed

infinite-persistence (Migrated from github.com) requested changes 2021-09-23 07:10:14 +02:00

ui/component/copyableStreamkey/view.jsx

infinite-persistence (Migrated from github.com) commented

2021-09-23 07:10:07 +02:00

Copy link

Issues:

The "Show" button didn't work for me.
This is still not the React-way -- normally, we don't directly tweak the DOM.
The use of <form> is weird, as we are not submitting anything.

Take a look at how to use useState.

A rough guide:

We probably don't need <form> and type=password
- Without password, you don't have to change the way you perform the copy anymore.
Create a state, e.g. showKey
Replace copyable with asterix if state is hidden
- e.g. value={showKey ? copyable || '' : '***')
  - Can make the asterix the same length if you want.

Issues: - The "Show" button didn't work for me. - This is still not the React-way -- normally, we don't directly tweak the DOM. - The use of `<form>` is weird, as we are not submitting anything. Take a look at how to use `useState`. A rough guide: - We probably don't need `<form>` and `type=password` - Without `password`, you don't have to change the way you perform the copy anymore. - Create a state, e.g. `showKey` - Replace `copyable` with asterix if state is hidden - e.g. `value={showKey ? copyable || '' : '***')` - Can make the asterix the same length if you want.

GlobalGamer2015 (Migrated from github.com) reviewed 2021-09-23 07:45:52 +02:00

ui/component/copyableStreamkey/view.jsx

GlobalGamer2015 (Migrated from github.com) commented

2021-09-23 07:45:51 +02:00

Copy link

Button is fixed.
I'm not using Dom, I'm using what was already in the file itself.
The <form> is being used to hold the main reference instead of a single FormField item.

Button is fixed. I'm not using Dom, I'm using what was already in the file itself. The `<form>` is being used to hold the main reference instead of a single FormField item.

infinite-persistence (Migrated from github.com) reviewed 2021-09-23 08:17:38 +02:00

ui/component/copyableStreamkey/view.jsx

infinite-persistence (Migrated from github.com) commented

2021-09-23 08:17:38 +02:00

Copy link

The ref was originally used to highlight the text (i.e. .select()) when being copied, which we couldn't do via React.
By default, we don't alter text directly that way. Use states.
<form> is not the right tag for that use-case. A <div> would probably do.

Merging

It works, and the changes are localized in 1 component, so good to go for now. Thanks!

- The ref was originally used to highlight the text (i.e. `.select()`) when being copied, which we couldn't do via React. - By default, we don't alter text directly that way. Use states. - `<form>` is not the right tag for that use-case. A `<div>` would probably do. ## Merging It works, and the changes are localized in 1 component, so good to go for now. Thanks!

infinite-persistence commented

2021-09-23 15:51:20 +02:00

(Migrated from github.com)

Copy link

Can we show you some appreciation?

Can we show you some [appreciation](https://lbry.com/faq/appreciation)?

Reviewers

No reviewers

infinite-persistence

Labels

Clear labels

Work is part of a proposal

Beamer is waiting on you!

Discuss this issue at the next planning meeting, then remove this label

Welcome to Hacktoberfest

No knowledge of the existing code required

level: 2

Some knowledge of the existing code is recommended

level: 3

Significant knowledge of the existing code is recommended

level: 4

Intimate knowledge of the existing code is recommended

merge when green

needs: exploration

Solution unclear, needs research

needs: grooming

Issue needs to be groomed before work can start

needs: priority

Priority level needs to be defined

needs: repro

Issue needs step-by-step instructions on how to reproduce in latest code

needs: tech design

Needs technical design signoff before implementation can begin

Issue is blocking release, do ASAP

priority: high

Work needs to be moved into sprint ASAP

priority: low

Work should be done but can stay in the backlog for now

priority: medium

Work needs to be done within 2-3 sprints

protocol dependent

recsys

redesign

general redesign not prioritize for anyone release

regression

resilience

sdk dependent

Requires work on lbry-sdk repo

Tom's Wishlist

trending

type: bug

Existing functionality is wrong or broken

type: discussion

A conversation. No specific changes requested

type: improvement

Existing (or partially existing) functionality needs to be changed

type: new feature

New functionality that does not exist yet

type: refactor

Minimal user-visible changes, but significant internal work

type: task

Either work that's not related to the code, or a small chore that does not fit into other categories

type: testing

Solution needs additional user testing

unplanned

Work that was not planned into the spirnt, took priority over planned work

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: LBRYCommunity/lbry-desktop#7127

Reference in a new issue

Repository

LBRYCommunity/lbry-desktop

Title

Body

No description provided.

Delete branch "master"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?