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

Issue 43571002: sao: add missing tooltips for toolbars' buttons and xxx2M fields (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 months, 2 weeks ago by fmorato
Modified:
5 months, 4 weeks ago
Reviewers:
pokoli, ced, reviewbot
Visibility:
Public.

Description

Patch Set 1 #

Total comments: 7

Patch Set 2 : Implement cedk's suggestions and explain data-original-title #

Patch Set 3 : Finish call gettext only once per button. Set bookmark button tooltip placement top #

Total comments: 7

Patch Set 4 : Use single variable title and set placement to auto #

Patch Set 5 : Replace tooltip creation by function #

Total comments: 9
Unified diffs Side-by-side diffs Delta from patch set Stats (+80 lines, -45 lines) Patch
M src/common.js View 1 2 3 4 2 chunks +10 lines, -3 lines 6 comments Download
M src/screen.js View 1 2 3 4 5 chunks +12 lines, -12 lines 3 comments Download
M src/tab.js View 1 2 3 4 4 chunks +7 lines, -11 lines 0 comments Download
M src/view/form.js View 1 2 3 4 11 chunks +49 lines, -16 lines 0 comments Download
M src/view/tree.js View 1 2 3 4 1 chunk +2 lines, -3 lines 0 comments Download

Messages

Total messages: 17
fmorato
11 months, 2 weeks ago (2017-10-06 19:34:03 UTC) #1
reviewbot
flake8 OK URL: https://codereview.tryton.org/43571002
11 months, 2 weeks ago (2017-10-06 19:40:53 UTC) #2
ced
https://codereview.tryton.org/43571002/diff/1/src/screen.js File src/screen.js (right): https://codereview.tryton.org/43571002/diff/1/src/screen.js#newcode49 src/screen.js:49: 'aria-label': Sao.i18n.gettext('Search') Would be better to ensure we use ...
11 months, 2 weeks ago (2017-10-06 22:37:53 UTC) #3
fmorato
Implement cedk's suggestions and explain data-original-title
11 months, 2 weeks ago (2017-10-07 16:26:08 UTC) #4
reviewbot
flake8 OK URL: https://codereview.tryton.org/43571002
11 months, 2 weeks ago (2017-10-07 16:46:08 UTC) #5
fmorato
Finish call gettext only once per button. Set bookmark button tooltip placement top
11 months, 2 weeks ago (2017-10-07 17:04:39 UTC) #6
reviewbot
flake8 OK URL: https://codereview.tryton.org/43571002
11 months, 2 weeks ago (2017-10-07 17:17:56 UTC) #7
pokoli
https://tryton-rietveld-hrd.appspot.com/43571002/diff/40001/src/screen.js File src/screen.js (right): https://tryton-rietveld-hrd.appspot.com/43571002/diff/40001/src/screen.js#newcode54 src/screen.js:54: .data('toggle', 'tooltip') Probably we should create a function to ...
10 months, 3 weeks ago (2017-11-02 12:15:50 UTC) #8
ced
https://codereview.tryton.org/43571002/diff/40001/src/screen.js File src/screen.js (right): https://codereview.tryton.org/43571002/diff/40001/src/screen.js#newcode54 src/screen.js:54: .data('toggle', 'tooltip') On 2017/11/02 12:15:50, pokoli wrote: > Probably ...
10 months ago (2017-11-21 22:55:51 UTC) #9
fmorato
On 2017/11/21 22:55:51, ced wrote: > https://codereview.tryton.org/43571002/diff/40001/src/screen.js > File src/screen.js (right): > > https://codereview.tryton.org/43571002/diff/40001/src/screen.js#newcode54 > ...
8 months ago (2018-01-23 12:07:20 UTC) #10
fmorato
Use single variable title and set placement to auto
8 months ago (2018-01-23 12:10:51 UTC) #11
reviewbot
flake8 OK URL: https://codereview.tryton.org/43571002
8 months ago (2018-01-23 12:11:22 UTC) #12
pokoli
On 2018/01/23 12:07:20, fmorato wrote: > On 2017/11/21 22:55:51, ced wrote: > > https://codereview.tryton.org/43571002/diff/40001/src/screen.js > ...
8 months ago (2018-01-23 12:39:31 UTC) #13
fmorato
Replace tooltip creation by function
8 months ago (2018-01-23 15:20:28 UTC) #14
pokoli
https://codereview.tryton.org/43571002/diff/80001/src/common.js File src/common.js (right): https://codereview.tryton.org/43571002/diff/80001/src/common.js#newcode3336 src/common.js:3336: var default_options = { placement: 'auto' }; why not ...
8 months ago (2018-01-23 15:33:17 UTC) #15
reviewbot
flake8 OK URL: https://codereview.tryton.org/43571002
8 months ago (2018-01-23 15:35:17 UTC) #16
ced
8 months ago (2018-01-23 18:12:28 UTC) #17
https://codereview.tryton.org/43571002/diff/80001/src/common.js
File src/common.js (right):

https://codereview.tryton.org/43571002/diff/80001/src/common.js#newcode3339
src/common.js:3339: el.attr('aria-label', merged_options.title || '');
On 2018/01/23 15:33:17, pokoli wrote:
> I think title should be a required parameter and not part of options.

+1

https://codereview.tryton.org/43571002/diff/80001/src/common.js#newcode3339
src/common.js:3339: el.attr('aria-label', merged_options.title || '');
I do not like too much that 'aria-label' is set here because the name of the
method does not imply it.
So the code that uses it which remove the aria-label is very strange.

https://codereview.tryton.org/43571002/diff/80001/src/common.js#newcode3340
src/common.js:3340: return el.tooltip(merged_options);
As you are just calling tooltip with options, I do not see the point of this
method.
@pokoliy suggested a method because you were calling .data etc. but indeed it
was not needed.

https://codereview.tryton.org/43571002/diff/80001/src/screen.js
File src/screen.js (right):

https://codereview.tryton.org/43571002/diff/80001/src/screen.js#newcode63
src/screen.js:63: Sao.common.tooltip(this.but_bookmark, {title:
Sao.i18n.gettext('Bookmarks')});
80 cols

https://codereview.tryton.org/43571002/diff/80001/src/screen.js#newcode94
src/screen.js:94: Sao.common.tooltip(this.but_star, {title: function(){
This can not be set statically but in set_star function.
Sign in to reply to this message.

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