Tryton Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(53)

Issue 42931002: sao: Add settings

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 months, 1 week ago by pokoli
Modified:
6 months ago
Reviewers:
xcodinas, ced, reviewbot
Visibility:
Public.

Description

Patch Set 1 #

Total comments: 1

Patch Set 2 : Use localStorage for saving settings #

Patch Set 3 : Do not create duplicate settings label #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+75 lines, -10 lines) Patch
M CHANGELOG View 1 1 chunk +2 lines, -0 lines 0 comments Download
M index.html View 1 chunk +1 line, -0 lines 0 comments Download
M src/action.js View 1 1 chunk +1 line, -1 line 0 comments Download
M src/common.js View 1 2 chunks +2 lines, -2 lines 1 comment Download
M src/model.js View 1 1 chunk +1 line, -1 line 1 comment Download
M src/sao.js View 1 2 6 chunks +62 lines, -0 lines 4 comments Download
M src/screen.js View 1 1 chunk +1 line, -1 line 0 comments Download
M src/view/form.js View 1 2 chunks +2 lines, -2 lines 0 comments Download
M src/view/tree.js View 1 2 chunks +3 lines, -3 lines 0 comments Download

Messages

Total messages: 7
pokoli
8 months, 1 week ago (2017-12-13 15:46:38 UTC) #1
reviewbot
flake8 OK URL: https://codereview.tryton.org/42931002
8 months, 1 week ago (2017-12-13 16:11:30 UTC) #2
xcodinas
https://tryton-rietveld.appspot.com/42931002/diff/1/src/sao.js File src/sao.js (right): https://tryton-rietveld.appspot.com/42931002/diff/1/src/sao.js#newcode365 src/sao.js:365: li.append(jQuery('<label>', { Why not append directly the text?
8 months, 1 week ago (2017-12-13 16:18:24 UTC) #3
pokoli
Use localStorage for saving settings
8 months ago (2017-12-18 13:18:04 UTC) #4
pokoli
Do not create duplicate settings label
8 months ago (2017-12-18 13:33:44 UTC) #5
reviewbot
flake8 OK URL: https://codereview.tryton.org/42931002
8 months ago (2017-12-18 13:50:57 UTC) #6
ced
6 months ago (2018-02-13 18:05:42 UTC) #7
I'm wondering if we really need a menu entry for that. As it is quite technical,
we could maybe expect the user to edit the localStorage by himself.

https://codereview.tryton.org/42931002/diff/40001/src/common.js
File src/common.js (right):

https://codereview.tryton.org/42931002/diff/40001/src/common.js#newcode2958
src/common.js:2958: href: Sao.config.get('roundup.url'),
I think it is strange to be a configuration on client side. It should common
from the server probably.

https://codereview.tryton.org/42931002/diff/40001/src/model.js
File src/model.js (right):

https://codereview.tryton.org/42931002/diff/40001/src/model.js#newcode630
src/model.js:630: var limit = parseInt(Sao.config.get('limit') /
fnames_to_fetch.length,
80 cols

https://codereview.tryton.org/42931002/diff/40001/src/sao.js
File src/sao.js (right):

https://codereview.tryton.org/42931002/diff/40001/src/sao.js#newcode200
src/sao.js:200: var value = localStorage.getItem('sao_' + key);
I think it is better to use: tryton_sao_
sao is too short.

https://codereview.tryton.org/42931002/diff/40001/src/sao.js#newcode350
src/sao.js:350: });
As it is a static menu, is it really needed to clear it?

https://codereview.tryton.org/42931002/diff/40001/src/sao.js#newcode365
src/sao.js:365: //'class': 'form-check'
???

https://codereview.tryton.org/42931002/diff/40001/src/sao.js#newcode457
src/sao.js:457: jQuery('#user-settings').children().remove();
It is not linked to the server so it can always stay.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 3870a69