Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tokens: new style #918

Merged
merged 32 commits into from Aug 19, 2019

Conversation

@bpierre
Copy link
Member

commented Jul 11, 2019

Preview

How to test

Assuming everything is in the ~/projects directory (replace by your own).

First time

Clone this repo + Aragon client’s + aragonUI’s if not done already:

cd ~/projects
git clone git@github.com:aragon/aragon-apps.git
git clone git@github.com:aragon/aragon.git
git clone git@github.com:aragon/aragon-ui.git

Create a link for aragonUI (npm might tell you that it is already done):

cd ~/projects/aragon-ui
npm link

Getting updates

Here we are updating the branches + reinstalling the dependencies + linking aragonUI again.

cd ~/projects/aragon-ui
git checkout newstyle
git pull origin newstyle
npm install

cd ~/projects/aragon
git checkout newstyle
git pull origin newstyle
npm install
npm link @aragon/ui

cd ~/projects/aragon-apps/apps/token-manager/app
git checkout newstyle-tokens
git pull origin newstyle-tokens
npm install
npm link @aragon/ui

Running it

We can now start the two servers (the app + Aragon client). In one shell session (or tab in your terminal app):

cd ~/projects/aragon-apps/apps/token-manager/app
npm start

In another session (we are asking the client to load apps running locally):

cd ~/projects/aragon
env REACT_APP_ASSET_BRIDGE=local yarn start

Open http://localhost:3000/ in your browser.

Depends on

aragon/aragon-ui#391
aragon/aragon#826

@coveralls

This comment has been minimized.

Copy link

commented Jul 11, 2019

Coverage Status

Coverage remained the same at 97.914% when pulling f3749f0 on newstyle-tokens into a4f873e on newstyle-0.8.

@bpierre bpierre marked this pull request as ready for review Jul 18, 2019

@bpierre bpierre requested review from AquiGorka, sohkai and dizzypaty Jul 19, 2019

)
}
}

export default () => {
const { api, appState, connectedAccount, requestMenu } = useAragonApi()
const theme = useTheme()
const themeMode = useThemeMode()

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Jul 22, 2019

Member

I've been maintaining a consistent interface to what hooks expose, when it is a handler prepend it with handle -> handleDoSomething instead of just exposing the inner useState method to update the state var, which we usually prepend with set.
const { set: handleThemeModeChange } = useThemeMode()
What do you think?

This comment has been minimized.

Copy link
@bpierre

bpierre Jul 22, 2019

Author Member

I’ve been using the handleX less and less since we moved to hooks.

With class components, we couldn’t pass methods declared on the class prototype to event handlers, since we needed this to be set properly, so we often had these arrow functions being used as wrappers, and often directly setting the state or other things. It is also quite frequent to see these methods doing several things in reaction to a particular event coming from a children, making the “handle + initiator + event” a good name.

With hooks, I find it to be too different to apply the same convention: the callbacks we create are often focused on are often focused in one task, and reused in different cases (e.g. called from another hook), and calling a function named “handleClick” from something else than a click event doesn’t seem right… so I tend to go for a name that reflect “what it does”, rather than “what it should be called from” now. It’s also safe to pass directly to event handlers since we don’t have the prototype methods + this problem anymore.

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Jul 22, 2019

Member

With hooks, I find it to be too different to apply the same convention: the callbacks we create are often focused on are often focused in one task

const { handleNameChange } = useName()
const handleEffectOnNameChange = () => {
  /* side effect */
  handleNameChange()
}
<Component onChange={handleNameChange} />
<OtherComponentTriggersSideEffect onChange={handleEffectOnNameChange} />

Sometimes one function yes but we can implement in a way that does not take away from the handleX format no?

and reused in different cases (e.g. called from another hook), and calling a function named “handleClick” from something else than a click event doesn’t seem right… so I tend to go for a name that reflect “what it does”, rather than “what it should be called from” now

I don't think I'd call a method exported from a hook handleClick but I would pass it down to a component on the onClick handler

Using a "name that reflects what it does" is exactly what I want to use the X for, and just preprend it with handle makes it explicit, the method is the handler.

I'd rather we stick to a convention that's widely used throughout the code base instead of switching randomly to another one - again as with all team conventions, I call forth the team so that we can decide as a group cc/ @sohkai @2color

This comment has been minimized.

Copy link
@bpierre

bpierre Jul 22, 2019

Author Member

I’m not sure I follow 🤔 the handle prefix was used to indicate that a function was handling an event, but in your examples it is the other way? e.g. handleNameChange is not “handling the name change event”, which is how we usually use these prefixes.

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Jul 22, 2019

Member

Let me take a step backwards then:

const useName = () => {
const [name, setName]=useState('')
// either (which would make sense in a hook named more like useNameWithInput or what not
return { handleNameChange: ({ target: value }) => setName(value) }
// or depending on the interface that you may want to provide
return { handleNameChange: setName }
}

I do understand there is a difference between the two names and this is definitely an over-simplified example, but the idea is that instead of sending out a setX from a hook, we can continue using the handle prefix as before to refer to the method that will take action when/if the state var changes.

This comment has been minimized.

Copy link
@sohkai

sohkai Jul 29, 2019

Member

Not quite sure what the React community consensus might be, but for me the distinction for handleX is that it's going to handle an event or action from one of its children.

I think setX being returned from props is the original intention, and the API we return from a hook doesn't necessarily have to be paired with its internal state.

E.g.

const useName = () => {
const [name, setName]=useState('')
// either (which would make sense in a hook named more like useNameWithInput or what not
return { setNameFromEvent: ({ target: value }) => setName(value) }
// or depending on the interface that you may want to provide
return { setName }
}

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Jul 30, 2019

Member

a hook doesn't necessarily have to be paired with its internal state

Sometimes for sure, when it is a generic hook we expose, but when it is a hook directly related to a component (or component tree) I do think it makes sense to be explicit about the handlers's names.


const ResponsiveTable = styled(Table)`
margin-top: 16px;
const editLabel = useCallback(() => showLocalIdentityModal(address), [

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Jul 22, 2019

Member

Nitpick for consistency: all these handlers, could you update the names to handleActionsToDo?

This comment has been minimized.

Copy link
@bpierre

bpierre Aug 3, 2019

Author Member

If it can add something to the other discussion:

I don’t consider these to be event handlers, even if they happen to be used as such later on. When a function is meant to handle an event specifically, I usually name it using the handle prefix with the emitted event − not the action it triggers.

For example here, a handle-prefixed name I could use would be handleClick() (which would be a problem since they are three 😆), since they “handle a click”. But they don’t really contain anything about handling this specific click event, so it could be limiting to do that (e.g. if we want to call it from somewhere else at some point), and it would also make it less easy to understand what they do by just reading the name.

handleEditLabel wouldn’t work following the same logic, because the function doesn’t “handle an edit”, but kind of the opposite: it triggers an edit.

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Aug 7, 2019

Member

handleClickForEdit?

This comment has been minimized.

Copy link
@bpierre

bpierre Aug 11, 2019

Author Member

I don’t think we should limit it to handling clicks 😆 I think it is more important to name this function after what it does (“edit a label”), rather than its usage (being passed to onClick). There is nothing related to handling a click event in this function, it might be called from anywhere.

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Aug 12, 2019

Member

it might be called from anywhere

This is what I'm pointing at, it might, but it is not, it is being used in a click handler. When the methods are specific to an implementation it doesn't need a generic name, if/when it responds to some other type of event maybe the abstract name should be updated but for now, only one use case: click.

@AquiGorka
Copy link
Member

left a comment

Looking pretty :snazzy-emoji:, left a couple of comments. I think you removed AppLayout no? This was the only place where MenuButton was used, could you remove it too (and other Components that are not used anymore)?

@bpierre bpierre referenced this pull request Jul 24, 2019
@sohkai

sohkai approved these changes Jul 29, 2019

Copy link
Member

left a comment

Couple of small things, but otherwise let's merge (after #936 et al)

apps/token-manager/app/src/App.js Show resolved Hide resolved
apps/token-manager/app/src/components/InfoBoxes.js Outdated Show resolved Hide resolved
apps/token-manager/app/src/components/InfoBoxes.js Outdated Show resolved Hide resolved
/>
) : (
!isSyncing && (
<EmptyState onActivate={this.handleLaunchAssignTokensNoHolder} />

This comment has been minimized.

Copy link
@sohkai

sohkai Jul 29, 2019

Member

We should probably double check this and give it a light update @dizzypaty (super low priority; no normal user should ever see this empty state on the Tokens app)

sohkai added some commits Aug 11, 2019

@sohkai

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

@bpierre I've pushed a few commits that add a bit of polish and update various deprecated things in aragonUI. Easiest to review commit-by-commit.

@bpierre

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2019

@sohkai All good! 🏁 🙏

@sohkai

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

@bpierre I've applied a fix for group mode in 2d5df34 (test via this org).

However, the DataView layout is a bit funny when there's just one column:

Screen Shot 2019-08-11 at 5 36 43 AM

Is there a way we can align the column to the left?

@sohkai sohkai force-pushed the newstyle-tokens branch from 88ec435 to 1e47203 Aug 11, 2019

@bpierre

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2019

@sohkai Thanks! 🙏 🙏 🙏

I fixed the issue with DataView and single fields: aragon/aragon-ui#449

@AquiGorka

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

@sohkai and @bpierre the scrolling issue happens here too, is it something we plan on fixing for this iteration or can it be pushed to a second iteration
Ref

@sohkai

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

@AquiGorka Since it happens everywhere, and ideally this should come through an aragonUI upgrade, I think we can do it separately?

@AquiGorka AquiGorka changed the base branch from master to newstyle-0.8 Aug 16, 2019

sohkai added some commits Aug 16, 2019

@sohkai

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

@bpierre Pushed a couple commits, to squeeze out the final bits I noticed.

Adds an updated empty state that can be improved later (again, most users should never see this):

Screen Shot 2019-08-16 at 10 40 14 AM


Depending on how aragon/aragon-ui#486 goes I have another commit updating this app's usage of the ContextMenu.

sohkai added some commits Aug 18, 2019

@sohkai sohkai merged commit a63a92c into newstyle-0.8 Aug 19, 2019

2 of 5 checks passed

Travis CI - Branch Build Errored
Details
Travis CI - Pull Request Build Errored
Details
License Compliance FOSSA is analyzing this commit
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details

@sohkai sohkai deleted the newstyle-tokens branch Aug 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.