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

Issue 30161002: sao: Don't show warning dialog message as dialog title

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 1 week ago by pokoli
Modified:
4 days, 6 hours ago
Reviewers:
ced, reviewbot
Visibility:
Public.

Description

Patch Set 1 #

Total comments: 2

Patch Set 2 : Show title on title and remove content if no message #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+15 lines, -5 lines) Patch
M src/common.js View 1 2 chunks +14 lines, -4 lines 3 comments Download
M src/rpc.js View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 9
pokoli
1 month, 1 week ago (2017-03-16 15:45:34 UTC) #1
reviewbot
flake8 OK URL: https://codereview.tryton.org/30161002
1 month, 1 week ago (2017-03-16 16:01:27 UTC) #2
ced
https://tryton-rietveld.appspot.com/30161002/diff/1/src/common.js File src/common.js (right): https://tryton-rietveld.appspot.com/30161002/diff/1/src/common.js#newcode2692 src/common.js:2692: .css('white-space', 'pre-wrap')); I think title could be wrapped.
1 month, 1 week ago (2017-03-17 12:23:38 UTC) #3
pokoli
Show title on title and remove content if no message
1 month ago (2017-03-21 13:14:04 UTC) #4
pokoli
https://tryton-rietveld.appspot.com/30161002/diff/1/src/common.js File src/common.js (right): https://tryton-rietveld.appspot.com/30161002/diff/1/src/common.js#newcode2692 src/common.js:2692: .css('white-space', 'pre-wrap')); On 2017/03/17 12:23:38, ced wrote: > I ...
1 month ago (2017-03-21 13:15:18 UTC) #5
reviewbot
flake8 OK URL: https://codereview.tryton.org/30161002
1 month ago (2017-03-21 13:28:37 UTC) #6
ced
https://tryton-rietveld.appspot.com/30161002/diff/20001/src/common.js File src/common.js (right): https://tryton-rietveld.appspot.com/30161002/diff/20001/src/common.js#newcode2702 src/common.js:2702: dialog.body.remove(); Is it really needed to remove the body?
4 days, 21 hours ago (2017-04-19 21:09:54 UTC) #7
pokoli
https://tryton-rietveld.appspot.com/30161002/diff/20001/src/common.js File src/common.js (right): https://tryton-rietveld.appspot.com/30161002/diff/20001/src/common.js#newcode2702 src/common.js:2702: dialog.body.remove(); On 2017/04/19 21:09:54, ced wrote: > Is it ...
4 days, 7 hours ago (2017-04-20 11:40:40 UTC) #8
ced
4 days, 6 hours ago (2017-04-20 12:54:39 UTC) #9
https://tryton-rietveld.appspot.com/30161002/diff/20001/src/common.js
File src/common.js (right):

https://tryton-rietveld.appspot.com/30161002/diff/20001/src/common.js#newcode...
src/common.js:2702: dialog.body.remove();
On 2017/04/20 11:40:40, pokoli wrote:
> On 2017/04/19 21:09:54, ced wrote:
> > Is it really needed to remove the body?
> 
> Yes, because it is not used and then  an empty space is shown. The space is
> wrapped with two line separators and it seems that something it's missing. 

OK, I think we should come back to version 1, it is closer to what tryton does.
I think it will be source of future bug to remove part of the dialog.
Sign in to reply to this message.

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