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

Side by Side Diff: tests/io-toggle.html

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

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

Left:
Right:

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

Jump to:

View unified diff | Download patch

OLD	NEW
(Empty)
	1 <!DOCTYPE html>

	2 <html>

	3 <head>

	4 <meta charset="utf-8">

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

	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">

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

	9 <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 margin: 4px;

	12 padding: 8px;

	13 border: 1px solid silver;

	14 line-height: 0;

	15 }

	16 </style>

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

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

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

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

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

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

	23 </head>

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

	25 <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>

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

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

	29 </body>

	30 </html>

OLD	NEW

« js/io-toggle.js ('K') | « package.json ('k') | tests/io-toggle.js » ('j') | no next file with comments »