tests/io-toggle.html - Issue 29730644: Issue 6514 - IOToggle Custom Element

Delta Between Two Patch Sets: tests/io-toggle.html

Issue 29730644: Issue 6514 - IOToggle Custom Element (Closed)

Left Patch Set: Created March 26, 2018, 2:24 p.m.

Right Patch Set: applied latest required changes Created May 2, 2018, 12:13 p.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

Left: Side by side diff | Download
Right: Side by side diff | Download

LEFT	RIGHT
1 <!DOCTYPE html>	1 <!DOCTYPE html>

2 <html>	2 <html>

3 <head>	3 <head>

4 <meta charset="utf-8">	4 <meta charset="utf-8">

5 <title>Test io-toggle.js</title>	5 <title>Test io-toggle.js</title>

6 <link rel="stylesheet" href="../skin/common.css">	6 <link rel="stylesheet" href="../skin/common.css">
saroyanm 2018/04/25 17:05:01 Seems like The toggle buttons are not dependent on Seems like The toggle buttons are not dependent on most of the styles and scripts here. I suggest to remove all the scripts and styles that have nothing to do with current component, in order not to create a false assumption. a.giammarchi 2018/04/26 10:44:52 it is mandatory to test components UI together wit Show quoted text On 2018/04/25 17:05:01, saroyanm wrote: > Seems like The toggle buttons are not dependent on most of the styles and > scripts here. > > I suggest to remove all the scripts and styles that have nothing to do with > current component, in order not to create a false assumption. it is mandatory to test components UI together with common UI rules to be sure there are no interferences, IMO. This is also some sort of testing template for all occasions. Thomas Greiner 2018/04/26 17:00:52 Acknowledged. Show quoted text On 2018/04/26 10:44:52, a.giammarchi wrote: > On 2018/04/25 17:05:01, saroyanm wrote: > > Seems like The toggle buttons are not dependent on most of the styles and > > scripts here. > > > > I suggest to remove all the scripts and styles that have nothing to do with > > current component, in order not to create a false assumption. > > it is mandatory to test components UI together with common UI rules to be sure > there are no interferences, IMO. This is also some sort of testing template for > all occasions. Acknowledged. a.giammarchi 2018/04/27 10:59:05 I forgot to answer this Manvel point. The `IOEleme Show quoted text On 2018/04/25 17:05:01, saroyanm wrote: > Seems like The toggle buttons are not dependent on most of the styles and > scripts here. > > I suggest to remove all the scripts and styles that have nothing to do with > current component, in order not to create a false assumption. I forgot to answer this Manvel point. The `IOElement` class uses `ext.i18n` to simplify the inclusion of i18n text in other components too. It is not possible to test anything without other scripts, and AFAIK all of them are needed to have `ext.i18n` available and working. When we'll finally decide to have modules for these global dependencies it'd be easy to drop every unrelated script. Until we have these global dependencies, those scripts are needed.
7 <link rel="stylesheet" href="../skin/fonts.css">	7 <link rel="stylesheet" href="../skin/fonts.css">

8 <link rel="stylesheet" href="../skin/desktop-options.css">	8 <link rel="stylesheet" href="../skin/desktop-options.css">

	9 <link rel="stylesheet" href="../skin/io-toggle.css">

9 <style>	10 <style>
Thomas Greiner 2018/04/25 17:29:14 I know that it's just a test but I think it'd be b I know that it's just a test but I think it'd be best to stick to existing conventions such as separating styles from the HTML. That way we can also lint any test-related resources such as these styles. a.giammarchi 2018/04/26 10:44:52 If we make writing tests hard, we'll most likely w Show quoted text On 2018/04/25 17:29:14, Thomas Greiner wrote: > I know that it's just a test but I think it'd be best to stick to existing > conventions such as separating styles from the HTML. That way we can also lint > any test-related resources such as these styles. If we make writing tests hard, we'll most likely will to skip writing them. I think we should be as free as possible for tests, of course inside the limit of decency and readability. A file for a single fieldset unnecessary for anything else seems overkill to me. Thomas Greiner 2018/04/26 17:00:52 While it sounds reasonable at first glance, I'm no Show quoted text On 2018/04/26 10:44:52, a.giammarchi wrote: > If we make writing tests hard, we'll most likely will to skip writing them. > > I think we should be as free as possible for tests, of course inside the limit > of decency and readability. While it sounds reasonable at first glance, I'm not sure how much of a hurdle it really is to stay consistent with our coding style or are you suggesting that we should not review tests? Show quoted text > A file for a single fieldset unnecessary for anything else seems overkill to me. I was thinking more along the lines of having a CSS file for tests which would also include the body's background color. However, since having inline CSS is not against any rules, I'm quite relaxed about keeping it inline because I agree with your point that it's better to keep things simple. a.giammarchi 2018/04/27 10:59:05 I am not saying tests should not be reviewed, I am Show quoted text On 2018/04/26 17:00:52, Thomas Greiner wrote: > On 2018/04/26 10:44:52, a.giammarchi wrote: > > If we make writing tests hard, we'll most likely will to skip writing them. > > > > I think we should be as free as possible for tests, of course inside the limit > > of decency and readability. > > While it sounds reasonable at first glance, I'm not sure how much of a hurdle it > really is to stay consistent with our coding style or are you suggesting that we > should not review tests? > > > A file for a single fieldset unnecessary for anything else seems overkill to > me. > > I was thinking more along the lines of having a CSS file for tests which would > also include the body's background color. > > However, since having inline CSS is not against any rules, I'm quite relaxed > about keeping it inline because I agree with your point that it's better to keep > things simple. I am not saying tests should not be reviewed, I am saying tests might contain incredible hacks to be able to work stand alone and if we are not super relaxed about tests every component will ship in 3X the time it would need without tests. Thomas Greiner 2018/05/02 11:53:28 I think we agree on all points and just need to ag Show quoted text On 2018/04/27 10:59:05, a.giammarchi wrote: > I am not saying tests should not be reviewed, I am saying tests might contain > incredible hacks to be able to work stand alone and if we are not super > relaxed about tests every component will ship in 3X the time it would need > without tests. I think we agree on all points and just need to agree on how we want to balance those two. So I'm fine with going forward with this to finish this review and discussing how we want to write tests with Manvel and Winsley separately.
10 fieldset {	11 fieldset

	12 {

11 margin: 4px;	13 margin: 4px;

12 padding: 8px;	14 padding: 8px;

13 border: 1px solid silver;	15 border: 1px solid silver;

14 line-height: 0;	16 line-height: 0;

15 }	17 }

16 </style>	18 </style>

17 <script defer src="../polyfill.js"></script>	19 <script defer src="../polyfill.js"></script>

18 <script defer src="../ext/common.js"></script>	20 <script defer src="../ext/common.js"></script>

19 <script defer src="../ext/content.js"></script>	21 <script defer src="../ext/content.js"></script>

20 <script defer src="../common.js"></script>	22 <script defer src="../common.js"></script>

21 <script defer src="../i18n.js"></script>	23 <script defer src="../i18n.js"></script>

22 <script defer src="./io-toggle.js"></script>	24 <script defer src="./io-toggle.js"></script>

23 </head>	25 </head>

24 <body style="background:white;">	26 <body style="background:white;">

25 <fieldset><io-toggle onchange="console.log(event)"></io-toggle></fieldset>	27 <fieldset><io-toggle onchange="console.log(event)"></io-toggle></fieldset>
saroyanm 2018/04/25 17:05:01 Suggestion: I suggest not include console.logs in Suggestion: I suggest not include console.logs in current test in order to prevent this being copy pasted to the production, by accident. a.giammarchi 2018/04/26 10:44:52 fair enough, but please consider my answer to Thom Show quoted text On 2018/04/25 17:05:01, saroyanm wrote: > Suggestion: I suggest not include console.logs in current test in order to > prevent this being copy pasted to the production, by accident. fair enough, but please consider my answer to Thomas too. We should probably be easy-going with tests, IMO. Code reviews would easily spot a copy and paste mistake.
26 <fieldset><io-toggle checked></io-toggle></fieldset>	28 <fieldset><io-toggle checked></io-toggle></fieldset>

27 <fieldset><io-toggle disabled></io-toggle></fieldset>	29 <fieldset><io-toggle disabled></io-toggle></fieldset>

28 <fieldset><io-toggle checked disabled></io-toggle></fieldset>	30 <fieldset><io-toggle checked disabled></io-toggle></fieldset>

29 </body>	31 </body>

30 </html>	32 </html>

LEFT	RIGHT