Fix autocrypt ui #262
Fix autocrypt ui #262
Conversation
Jikstra
commented
Nov 1, 2018
Jikstra
commented
|
Fixes #195 |
|
|
||
| return e + (addDash ? ' - ' : '') + (addLineBreak ? '\n' : '') | ||
| }).join('') | ||
| } |
There was a problem hiding this comment.
Can we make this a normal function? No reason to be a class method since it doesn't use any class state (i.e. not using this).
There was a problem hiding this comment.
I don't care but i like to have it in a class just to have it grouped with where it belongs to. Also i think i already saw similiar stuff in the codebase. I don't really like to have a function in the same file floating around and you don't really know where it belongs to. If it's in a class and not a static function, you know you can only call it from inside (okay, prefix it with a _...). But its at least visually next to the stuff which uses it.
There was a problem hiding this comment.
Ok, do what you feel is best.
| ) | ||
|
|
||
| if (i !== 8) { | ||
| inputs.push(<div class='seperator' />) |
| } | ||
|
|
||
| render () { | ||
| const tx = window.translate | ||
| console.log(this.state) |
static/main.css
Outdated
| padding-bottom: 10px; | ||
| } | ||
|
|
||
| .InputTransferKey .seperator { |
static/main.css
Outdated
| text-align: center; | ||
| } | ||
|
|
||
| .InputTransferKey .seperator::after { |
| } | ||
|
|
||
| handleChange (event) { | ||
| handleChangeKey (event) { |
There was a problem hiding this comment.
can remove this TODO now :)
static/main.css
Outdated
| } | ||
|
|
||
| .InputTransferKey .seperator { | ||
| text-align: center; |
There was a problem hiding this comment.
is this text-align: center needed? I wonder if we could create then a class .centered or use styled-components or something..
There was a problem hiding this comment.
It's only needed to have to "-" exactly in the middle of the 10px grid.
static/main.css
Outdated
| } | ||
|
|
||
| .InputTransferKey .seperator::after { | ||
| content: '-'; |
There was a problem hiding this comment.
instead can we use - in the div? I find this content property is only really useful in cases where you don't have access to generating the dom directly, but in this case we do
There was a problem hiding this comment.
Yup sounds good to me.
| // TODO: insert - automatically in between every 4 characters | ||
| // TODO: lint the value for correct setup message format | ||
| this.setState({ key: event.target.value, message: false }) | ||
| if (event.target.value < 0 || event.target.value > 9999) { |
There was a problem hiding this comment.
What happens when the user inputs a string like asdf?
If the value isn't a number, we should return false also. So here:
var value = Number(event.target.value)
if (isNaN(value) || value < 0 || value > 9999) return false
let updatedkey = this.state.key
updatedkey[Number(event.target.id)] = value
this.setState({ key: updatedkey })| onChange={this.handleChange} | ||
| value={this.state.key} | ||
| /> | ||
| <div class='InputTransferKey'> |
There was a problem hiding this comment.
what if we use styled-components here instead?
There was a problem hiding this comment.
Can we make a separate issue for styled-components and gather up points in the code where it's useful?
There was a problem hiding this comment.
Sure. all of the wrappers that can be packaged as components
|
@Jikstra rebase + meeeeerrrrrge! :) |
Jikstra
force-pushed
the
fix_autocrypt_ui
branch
from
6ce1ac6
to
27b0ece
Compare
|
Hmm github doesn't really notice that the conflicts are already solved... can't merge it :( |
Rework autocrypt transfer key uis * Add line breaks and spaces between dashes on autocrypt key * Implement "splitted input with dash seperated" form for typing in transfer key
