rewrite with react #48
Conversation
|
qa notes:
all in all, i think this is a solid direction to take things and i'm thrilled @gasolin that you took the initaitive! cc @mbeacom to see what he thinks ! |
|
QA note 2 the |
|
The loading is faster now. Check the screencast @mbeacom how do you think? |
|
Hi @mbeacom @owocki , I saw the bounty universe is in alpha and it looks like a perfect fit with this extension. Could we have a quick talk to decide if we want to implement the next features based on this react rewrite? |
|
@gasolin Sounds good! When were you thinking? |
|
@owocki Sure! |
|
@owocki your proposed time doesn't fit well to me(12am ~ 6am local time), so I'll write a GIP for the improvement proposal https://github.com/gitcoinco/GIPs instead and we can see if it make sense. |
pick from 57767a7
…ction to display funding/claim button correctly
ghost
assigned 
public/script/pageload/body.js
Outdated
|
|
||
| //injects various information on the page | ||
| var body = function(url) { | ||
| var isOnGitHubcom = url.indexOf('://github.com') !== -1 && url.indexOf('://github.com') < 15; |
There was a problem hiding this comment.
May I know the significance of the magic number 15?
There was a problem hiding this comment.
not sure why this number comes out.
Since we already do some protection via extension manifest, lets remove it
| //run callbacks from event | ||
| for (var i = 0; i < callbacks.length; i++){ | ||
| var callback = callbacks[i]; | ||
| callback(event); | ||
| } | ||
|
|
||
| }, false); | ||
|
|
||
| injectWeb3Context(); |
There was a problem hiding this comment.
Why is this function call removed?
There was a problem hiding this comment.
since we don't need to show web3 related info in extension now
src/components/Bot.js
Outdated
| import React from 'react'; | ||
| import robot from './bot.png'; | ||
|
|
||
| export function Bot() { |
There was a problem hiding this comment.
Looks like we're not rendering the Bot component in App.
- May I know why?
- If we don't need it, can we remove this component file?
There was a problem hiding this comment.
it was made to mimic the old ui. I disable it to let extension focus on the main content. would remove it
src/components/Header.js
Outdated
| import React from 'react'; | ||
| import banner from './header.png'; | ||
|
|
||
| export function Header() { |
There was a problem hiding this comment.
Ditto, why isn't the Header component used in App component?
There was a problem hiding this comment.
same reason, would remove it
| </div> | ||
| <h5>Funded Issues</h5> | ||
| <input type='text' id='search_bar' value={this.state.keyword} placeholder='Search for keywords..' onChange={this.handleChange} onKeyDown={this.handleKeyDown} /> | ||
| <button id="search-button" style={searchBtnStyle} onClick={this.handleClick} className='btn btn-sm btn-primary js-details-target gitcoin_button'>Search</button> |
There was a problem hiding this comment.
I think the search button should be disabled if the query is empty.
What do you think?
There was a problem hiding this comment.
when query is empty, press enter will show non-filtered result
src/components/History.js
Outdated
| </thead> | ||
| <TableNodes {...this.state} /> | ||
| </table> | ||
| <a target='new' href='https://gitcoin.co/explorer' rel='noopener noreferrer'>View More >> </a> |
There was a problem hiding this comment.
I think when a user searches for something and if there aren't any results, the 'View more' link shouldn't be rendered.
Thoughts?
|
@gasolin Thanks for addressing my comments and apologies for taking so long on this one. LGTM 👍 |
related to #47