added share option #510
added share option #510
Conversation
| @@ -26,4 +46,87 @@ const ShareQueue = (props) => { | |||
| ); | |||
| }; | |||
|
|
|||
| const ShareQueue = (props) => { | |||
| const [open, setOpen] = useState(false); | |||
There was a problem hiding this comment.
The component gets re-rendered in the background when we make an API call to the server in the background. (See here)
This causes the popper to close. You can observe this by opening it, and waiting for less than 10s. If it's trivial to get around this, you can try, otherwise leave it as is. This can be handled later more elegantly after #507.
There was a problem hiding this comment.
I used memo() even then the component is rendering.
There was a problem hiding this comment.
Alright, you mean that it is not helping, right? If not, let's remove the memo()
There was a problem hiding this comment.
I noticed this too, let me see if I can figure out why.
There was a problem hiding this comment.
It's due to us polling for updates, some logic like this would work, atleast it looks like there are no problems when I tried it.
| const [open, setOpen] = useState(false); | |
| let prevData; | |
| const update = useCallback(() => { | |
| clearTimeout(timeoutId); | |
| requestMaker(QueueRequestFactory.get(queueId)).then((data) => { | |
| if (data && (JSON.stringify(prevData) !== JSON.stringify(data))) { | |
| setTokens(data.tokens); | |
| ... |
Basically comparing old data with new one and only changing state if needed. There might be some inbuilt method that react provides for this, not really sure yet.
There was a problem hiding this comment.
A positive side effect of #507 is that, then one component will fire an action (refresh button, periodic refresh service), and only the component that subscribes to this data gets re-rendered.
Then we would not have this problem. If we can wait till #507 is done, we can leave it as is and merge this PR. I don't see an urgent need to update main site. Else if we are going to do some quick-fix, let us leave a comment here explaining what and why, so that we remove it post #507 .
There was a problem hiding this comment.
#507 has its first component ready. To do all the components it will take more time or hands on deck. It should be a priority since it's gonna make a lot of things easier.
There was a problem hiding this comment.
@mradenovic Can you do one component and let us finalise and merge that. Once that's done, me, @maaverik and others will migrate the remaining components taking your PR as a reference.
I think we can do a lot of other things in the same way. You lead the way and do one sample. Remaining repetitive work, we will take up. ✌️
There was a problem hiding this comment.
Absolutely @daltonfury42 , Anything for me?
|
Updated!!!! 💯 |
There was a problem hiding this comment.
This is a great start, kudos @prabureddy. We can brainstorm and improve it further down the line. A small addition I thought would be nice is if we could add a small border or a gradient difference between the dropdown arrow and the "Copy Queue Link" text, to make it really obvious that the arrow does something else, like we see in the fork and star button vs count in github.
simplq/.eslintrc.json
Outdated
| @@ -19,6 +19,7 @@ | |||
| "react/destructuring-assignment": 0, | |||
| "prefer-destructuring": 0, | |||
| "import/no-named-as-default": 0, | |||
| "dot-notation": 0 | |||
| "dot-notation": 0, | |||
| "react/jsx-props-no-spreading": 0 | |||
There was a problem hiding this comment.
Can we avoid this and explicitly pass down props?
I was checking, and props spreading does affect readability.
If there is a good reason you need to do this in the PR, we can disable it inline where you are using it, but try to avoid.
There was a problem hiding this comment.
Yes, removed (react/jsx-props-no-spreading) and passed down props. Now it is readable.
2. removed react/jsx-props-no-spreading
Resolves #509