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

procedurally generate background based on signer key #2233

Merged
merged 16 commits into from
Sep 22, 2016
Merged

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Sep 21, 2016

https://github.com/ethcore/parity/issues/2228

Next up -

  • as part of setup, user can have some way of selecting (signer token setup with initial token)
  • allow changing of the background in settings (signer keys stays the same, postfix for key changes)
  • need to adjust Modals (look slightly out of place)
  • ParityBar (dapp overlay) needs to get the same styling as modals (at least container overlays)

Caveats -

  • currently with each refresh a new background will be used. This is on purpose (adding the current timestamp as part of the token sha3) - it allows us to go through as many backgrounds as possible and see how things fit together from a UI perspective.

In action -

parity 2016-09-22 00-57-15
parity 2016-09-22 00-56-52
parity 2016-09-22 00-56-32
parity 2016-09-22 00-56-15
parity 2016-09-22 00-55-56
parity 2016-09-22 00-55-38
parity 2016-09-22 00-55-26
parity 2016-09-22 00-55-08
parity 2016-09-22 00-54-53
parity 2016-09-22 00-54-40
parity 2016-09-22 00-54-22

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. M-ui labels Sep 21, 2016
@parity-cla-bot
Copy link

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

Many thanks,

Ethcore CLA Bot

@jacogr jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 21, 2016
Copy link
Contributor

@derhuerst derhuerst left a comment

Choose a reason for hiding this comment

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

Please think about the naming of ProcBackground. :)

import GeoPattern from 'geopattern';
import React, { Component, PropTypes } from 'react';

export default class ProcBackground extends Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not happy with the name ProcBackground. I can't tell what Proc means if I don't know what it's about. I'd prefer e.g. GeneratedBackground, PatternBackground or – similar to IdentityIconIdentityBackground.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have renamed it - agreed with you on that actually, it is now ParityBackground, exactly what the function is.

render () {
const { children } = this.props;
const { backgroundImage } = this.state;
const style = { backgroundImage, width: '100%', height: '100%' };
Copy link
Contributor

Choose a reason for hiding this comment

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

Setting width and height to 100% may be dangerous in some places? Are they 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.

Sadly, yes. With the height we only stretch to child height, which means the image may just cover parts of the screen. Other option may be to set the child heights as/when required.

Copy link
Collaborator

@tomusdrw tomusdrw Sep 21, 2016

Choose a reason for hiding this comment

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

Why not: position: absolute; top: 0; bottom: 0; left: 0; right: 0;?

<div>
  <div style={style}></div> 
  { children }
</div>

Could be even a separate ReactDOM.render call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed that. The height was wrong (re-usable components), so if needed (which is one place), the className can set it. Actually in that place Application/Container the className already does.

@@ -28,11 +28,11 @@ muiTheme.raisedButton.primaryTextColor = 'white';
muiTheme.snackbar.backgroundColor = 'rgba(255, 30, 30, 0.9)';
muiTheme.snackbar.textColor = 'rgba(255, 255, 255, 0.9)';
muiTheme.tabs = lightTheme.tabs;
muiTheme.tabs.backgroundColor = 'rgb(65, 65, 65)';
muiTheme.tabs.backgroundColor = 'none'; // 'rgba(65, 65, 65, 0.75)';
Copy link
Contributor

Choose a reason for hiding this comment

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

Just taste, but I think it looks better without transparency. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we need to get it in mostly as-in and see what bugs us. There will need to be tweaks to make everything fit and look optimal.

Copy link
Contributor Author

@jacogr jacogr Sep 21, 2016

Choose a reason for hiding this comment

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

Adjusted it slightly. There is method behind the madness, with some transparency (0.7 now, up from 0.5) the background colors bleed through so it looks consistent. The trick is to get it just enough so it bleeds enough, but not too much that it is distracting.

As of now, quite happy as an itial stab. Will need to tweak as things look out of place, but for that need testing. (Plus, meeting with UX guy on Monday, we may get some help from a pure styling perspective, so not going to overthink it.)

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Sep 21, 2016
@derhuerst derhuerst added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 22, 2016
@jacogr jacogr merged commit 89de22d into js Sep 22, 2016
@jacogr jacogr deleted the jg-proc-background branch September 23, 2016 16:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants