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

Stop flickering + added loader in AddressSelector #4149

Merged
merged 2 commits into from
Jan 13, 2017
Merged

Conversation

ngotchac
Copy link
Contributor

Fixes #4103

@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Jan 12, 2017
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 1b290a2 on ng-address-flickering into ** on master**.

onBlur={ this.handleInputBlur }
onFocus={ this.handleInputFocus }
onChange={ this.handleChange }
onBlur={ this.handleInputBlur }
Copy link
Contributor

Choose a reason for hiding this comment

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

Alphabetical order is fine, just aligning style everywhere, no extra breaks needed. (I would rather not introduce more styling preferences yet again, props is still all-over the place.)


ref={ this.setInputRef }
/>
ref={ this.setInputRef }
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, extra "it looks pretty to some" break.

render () {
const size = (this.props.size || 2) * 60;
const { className, size } = this.props;
const _size = size * 60;
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer to keep _name to function params where supplied. So may make more sense as calculatedSize.

return (
<Loading
className={ styles.loader }
size={ 0.5 }
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to override the defaults? How does this work for consistency across the full UI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really really big. This size fills perfectly the input

@@ -76,7 +76,7 @@ class InputAddress extends Component {

const props = {};

if (!readOnly && !disabled) {
if (!disabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this changed? The whole readOnly/disabled is an issue overall for this component, seems that was added due to some issue somewhere and now removed due to some issue here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, added here for contract params - ethcore/parity@d16ab5e

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just tested, fine as it is. It wasn't properly adding the right underline line on focus, and would break the tabbing.

@jacogr
Copy link
Contributor

jacogr commented Jan 12, 2017

  1. Accounts column has a horizontal scrollbar, seems like extra space ended up in that column. It is the first time I'm noticing this, possibly not introduced here, but breaks the layout.

  2. Headings are significantly larger than modal headings, should really be the same. (Very visible not with the 75% overlay - same would go for input as well). Once again not quite new.

  3. Padding around modal top is not the same as on the sides, should be aligned. (If one is bigger it is typically top/bottom not left/right, this is reversed)

(Mentioning all of these here since this does fix dialog issues and would like to get it right so we can move on)

@ngotchac
Copy link
Contributor Author

  1. Not sure that I can reproduce it. Could reproduce a horizontal scroll bar on very very narrow screen. Fixed it though.

Fixed to other comments.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling adc377a on ng-address-flickering into ** on master**.

@jacogr
Copy link
Contributor

jacogr commented Jan 13, 2017

  1. Weird. Shows up for me each time. Will re-look to see if it still happens with the current version.

@derhuerst
Copy link
Contributor

@jacogr

  1. cannot replicate
  2. definitely agree, but wouldn't necessarily change it now. seems out of scope of this pr.

@ngotchac
Copy link
Contributor Author

@derhuerst 2. Has been fixed ;)

@jacogr
Copy link
Contributor

jacogr commented Jan 13, 2017

1 is due to this https://github.com/ethcore/parity/issues/4160

@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 13, 2017
@jacogr jacogr merged commit 57ce845 into master Jan 13, 2017
@jacogr jacogr deleted the ng-address-flickering branch January 13, 2017 14:52
arkpar pushed a commit that referenced this pull request Jan 16, 2017
* Stop UI flickering + added loader to AddressSelector #4103

* PR Grumbles
gavofyork pushed a commit that referenced this pull request Jan 16, 2017
* verification: check if server is running (#4140)

* verification: check if server is running

See also ethcore/email-verification#67c6466 and ethcore/sms-verification#a585e42.

* verification: show in the UI if server is running

* verification: code style ✨, more i18n

* fix i18n key

* Optimized hash lookups (#4144)

* Optimize hash comparison

* Use libc

* Ropsten fork detection (#4163)

* Stop flickering + added loader in AddressSelector (#4149)

* Stop UI flickering + added loader to AddressSelector #4103

* PR Grumbles

* Add a password strength component (#4153)

* Added new PasswordStrength Component

* Added tests

* PR Grumbles

* icarus -> update, increase web timeout. (#4165)

* icarus -> update, increase web timeout.

* Fix estimate gas

* Fix token images // Error in Contract Queries (#4169)

* Fix dapps not loading (#4170)

* Add secure to dappsreg

* Remove trailing slash // fix dapps
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants