Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Allow tagging of Accounts, Addresses and Contracts #2697

Closed
wants to merge 1,371 commits into from

Conversation

ngotchac
Copy link
Contributor

(#2643)

This PR adds tagging capabilities to Accounts, Addresses and Contracts. Multiple can be added per item, and these can be searched. They can also be sorted by name or by tag.

Currently, the search accepts multiple tokens, that are joined with OR ; eg. if item 1 has tag foo and item 2 has tag bar, as search for the two tokens foo and bar will display item 1 and 2.
This can be changed easily to an AND behavior.

See it in action here: https://youtu.be/3UA-QI0G_UQ

jacogr and others added 30 commits October 1, 2016 20:40
update ABI json to latest deployed versions
tokenreg dapp fixes for non-null returns
signaturereg registered, remove hardcoding
* js:
  signaturereg registered, remove hardcoding
  fixes for non-null returns
  update ABIs to latest deployed versions
  update Morden registry address (#2417)
  using arc (#2420)
  asterisk space
  removed redundant memcopy
  Update gitlab-ci
  Fixing logs-receipt matching (#2403)
  fix broken beta compilation
  Fixing transaction queue (#2392)
  separate mod for tests
  bloom filter crate
@parity-cla-bot
Copy link

It looks like this contributor signed our Contributor License Agreement. 👍

Many thanks,

Ethcore CLA Bot

@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. M5-ui labels Oct 18, 2016
@jacogr
Copy link
Contributor

jacogr commented Oct 18, 2016

@ngotchac Could you create a new branch against master and cherry-pick your commits into that? This shows the whole old history since you originally branched from JS.

@@ -131,6 +132,16 @@ module.exports = {
]
}),
new HappyPack({
id: 'less',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anything that less would give us (for our usage), that PostCSS does not?

Look above at your less file, stock-standard CSS. (Well, you did have the & nested, which is indeed something I do miss from Less & Sass.) However in PostCss - https://github.com/postcss/postcss-nested

Copy link
Contributor

@jacogr jacogr Oct 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition, don't see where less is imported (package.json), but may just be default by happypack without anything required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed for using PostCSS, just wanted to try out.
And LESS is automatically loaded by Webpack, from the less and less-loader packages.

const { tags } = meta || [];
const onChange = (chips) => this.onMetaChange('tags', chips);

return (<ChipInput
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return (
  <ChipInput ... />
);

@@ -17,6 +17,8 @@
import React, { Component, PropTypes } from 'react';
import ContentClear from 'material-ui/svg-icons/content/clear';
import ContentSave from 'material-ui/svg-icons/content/save';
// import ChipInput from 'material-ui-chip-input';
import ChipInput from 'material-ui-chip-input/src/ChipInput';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for this? Just curious? (Would remove the original if not needed)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's because the published material-ui-chip-input package didn't have all the features needed. Thus I forked it and used my fork here. I made a PR here : TeamWertarbyte/material-ui-chip-input#29

Now that it's merged, I have to try out using the published npm package again.

@jacogr
Copy link
Contributor

jacogr commented Oct 18, 2016

Overall, some nitpick comments & the branching issues. (One-time only) I would probably have a "default sort order" as well, i.e. what it does currently. Unless we default now to one of the new ones.

@jacogr jacogr added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 18, 2016
}

:local(.tag) {
font-size: 0.8rem;
Copy link
Contributor

@jacogr jacogr Oct 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would go for 0.75em since it is more-or-less consistent with elsewhere where we use smaller fonts. (And should actually be a variable so we can stop guessing everywhere - with the BasicToken stuff have used PostCSS variables, it does what it says on the tin. See my Less comment around explosion of "stuff to do css")

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling fd5c206 on ng-accounts-tagging into * on master*.

@ngotchac
Copy link
Contributor Author

See new PR #2712

gavofyork pushed a commit that referenced this pull request Oct 19, 2016
* Added tag to the editMeta Modal (#2643)

* Added Tags to ui and to contract/address/account Header (#2643)

* Added tags to summary (#2643)

* Added Search capabilities to contracts/address book/accounts from tokens
(#2643)

* fixes eslint

* Using Chips/Tokens for search (#2643)

* Add search tokens, clickable from List (#2643)

* Add sort capabilities to Accounts / Addresses / Contracts (#2643)

* Fixes formatting issues + state updates after component unmount bug
(#2643)

* Remove unused import

* Small fixes for PR #2697

* Added default sort order for Contracts/Addresses/Accounts

* Using official `material-ui-chip-input` NPM package

* Removed LESS from webpack
@ngotchac ngotchac deleted the ng-accounts-tagging branch October 19, 2016 15:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A5-grumble 🔥 Pull request has minor issues that must be addressed before merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants