fix: issue 39 #42
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,7 @@ | |
| <description>Simple Timer for Vaadin Flow</description> | ||
|
|
||
| <properties> | ||
| <vaadin.version>14.11.13</vaadin.version> | ||
| <maven.compiler.source>1.8</maven.compiler.source> | ||
| <maven.compiler.target>1.8</maven.compiler.target> | ||
| <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | ||
|
|
@@ -128,6 +128,34 @@ | |
| <type>jar</type> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>com.vaadin</groupId> | ||
| <artifactId>vaadin-testbench</artifactId> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>com.vaadin</groupId> | ||
| <artifactId>license-checker</artifactId> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.hamcrest</groupId> | ||
| <artifactId>hamcrest-library</artifactId> | ||
| <version>1.3</version> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>io.github.bonigarcia</groupId> | ||
| <artifactId>webdrivermanager</artifactId> | ||
| <version>5.9.1</version> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>com.flowingcode.vaadin.test</groupId> | ||
| <artifactId>testbench-rpc</artifactId> | ||
| <version>1.3.0</version> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| </dependencies> | ||
|
|
||
| <build> | ||
|
|
@@ -223,6 +251,89 @@ | |
| </build> | ||
|
|
||
| <profiles> | ||
| <profile> | ||
| <id>it</id> | ||
| <build> | ||
| <plugins> | ||
| <plugin> | ||
| <groupId>org.eclipse.jetty</groupId> | ||
| <artifactId>jetty-maven-plugin</artifactId> | ||
| <version>${jetty.version}</version> | ||
| <configuration> | ||
| <scanIntervalSeconds>0</scanIntervalSeconds> | ||
| <supportedPackagings> | ||
| <supportedPackaging>jar</supportedPackaging> | ||
| </supportedPackagings> | ||
| <stopKey>${project.artifactId}</stopKey> | ||
| <stopPort>8081</stopPort> | ||
| </configuration> | ||
| <executions> | ||
| <execution> | ||
| <id>start-jetty</id> | ||
| <phase>pre-integration-test</phase> | ||
| <goals> | ||
| <goal>start</goal> | ||
| </goals> | ||
| </execution> | ||
| <execution> | ||
| <id>stop-jetty</id> | ||
| <phase>post-integration-test</phase> | ||
| <goals> | ||
| <goal>stop</goal> | ||
| </goals> | ||
| </execution> | ||
| </executions> | ||
| </plugin> | ||
| <plugin> | ||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-failsafe-plugin</artifactId> | ||
| <version>2.22.2</version> | ||
| <executions> | ||
| <execution> | ||
| <goals> | ||
| <goal>integration-test</goal> | ||
| <goal>verify</goal> | ||
| </goals> | ||
| </execution> | ||
| </executions> | ||
| <configuration> | ||
| <trimStackTrace>false</trimStackTrace> | ||
| <enableAssertions>true</enableAssertions> | ||
| <systemPropertyVariables> | ||
| <!-- Pass location of downloaded webdrivers to the tests --> | ||
| <webdriver.chrome.driver> | ||
| ${webdriver.chrome.driver} | ||
| </webdriver.chrome.driver> | ||
| </systemPropertyVariables> | ||
| </configuration> | ||
| </plugin> | ||
| <plugin> | ||
| <artifactId>maven-resources-plugin</artifactId> | ||
| <version>3.1.0</version> | ||
| <executions> | ||
| <execution> | ||
| <!-- Since the view class is defined in test, after compilation copy | ||
| it to the runtime classpath to ensure it gets visited by vaadin-maven-plugin --> | ||
| <id>copy-test-to-classes</id> | ||
| <phase>process-test-classes</phase> | ||
| <goals> | ||
| <goal>copy-resources</goal> | ||
| </goals> | ||
| <configuration> | ||
| <outputDirectory>${project.build.outputDirectory}</outputDirectory> | ||
| <resources> | ||
| <resource> | ||
| <directory>${project.build.testOutputDirectory}</directory> | ||
| </resource> | ||
| </resources> | ||
| </configuration> | ||
| </execution> | ||
| </executions> | ||
| </plugin> | ||
| </plugins> | ||
| </build> | ||
| </profile> | ||
|
|
||
| <profile> | ||
| <id>directory</id> | ||
| <build> | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -109,6 +109,10 @@ public void setDoubleDigitHours(final boolean doubleDigitHours) { | |
| getElement().setProperty("doubleDigitHours", doubleDigitHours); | ||
| } | ||
|
|
||
| public void setTargetId(final String targetId) { | ||
| getElement().setProperty("targetId", targetId); | ||
| } | ||
|
|
||
| /** Starts or stops the timer if it is already started */ | ||
| public void start() { | ||
| getElement().callJsFunction("start"); | ||
|
|
@@ -167,8 +171,8 @@ public CompletableFuture<BigDecimal> getCurrentTimeAsync() { | |
| * @return this registration, for chaining | ||
| */ | ||
| public Registration addCurrentTimeChangeListener( | ||
| PropertyChangeListener listener, final long period, final TimeUnit periodUnit) { | ||
| final int millis = (int) Math.min(periodUnit.toMillis(period), Integer.MAX_VALUE); | ||
| if (listener == null) { | ||
| listener = ev -> {}; | ||
| } | ||
|
|
@@ -202,7 +206,7 @@ public boolean isVisible() { | |
| } | ||
|
|
||
| @Override | ||
| public void setVisible(final boolean visible) { | ||
| getStyle().set(DISPLAY, visible ? INLINE : "none"); | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -110,27 +110,30 @@ Polymer({ | |
| type: Boolean, | ||
| value: false | ||
| }, | ||
| targetId: { | ||
| type: String, | ||
| value: null, | ||
| }, | ||
| /** | ||
| * Time the timer has spent running since it was started | ||
| */ | ||
| _elapsed: { | ||
| type: Number, | ||
| value: 0 | ||
| }, | ||
|
|
||
| _formattedTime: { | ||
| type: String, | ||
| value: '0', | ||
| observer: "_updateTarget", | ||
| } | ||
| }, | ||
|
|
||
| ready: function() { | ||
| if (this.currentTime===undefined) { | ||
| this.set('currentTime', this.countUp ? 0 : this.startTime); | ||
| } | ||
| this.set('_formattedTime', this._formatTime(this.currentTime.toString())); | ||
| }, | ||
|
|
||
| start: function() { | ||
|
|
@@ -199,5 +202,10 @@ Polymer({ | |
| hours = hours.toString().padStart(2, '0'); | ||
| } | ||
| return (this.hours ? hours + ':' : '') + (this.minutes || this.hours ? minutes + ':' : '') + seconds + (this.fractions ? ('.' + timeString[1].substring(0,2)) : '') | ||
| }, | ||
| _updateTarget: function(newValue, oldValue){ | ||
| if (document.getElementById(this.targetId)) { | ||
| document.getElementById(this.targetId).innerText = newValue; | ||
| } | ||
| } | ||
|
Comment on lines
+206
to
+210
There was a problem hiding this comment. 🛠️ Refactor suggestion Avoid direct manipulation of the global The Consider using events to communicate with the outside world or exposing a property that can be bound by the parent component. Here's an example using a custom event: Refactored _updateTarget: function(newValue, oldValue){
- if (document.getElementById(this.targetId)) {
- document.getElementById(this.targetId).innerText = newValue;
- }
+ this.dispatchEvent(new CustomEvent('simple-timer-update', {
+ detail: { formattedTime: newValue },
+ bubbles: true,
+ composed: true
+ }));
}Usage in the parent component: <simple-timer id="myTimer"></simple-timer>
<span id="timeDisplay"></span>
<script>
const timer = document.getElementById('myTimer');
timer.addEventListener('simple-timer-update', function(event) {
document.getElementById('timeDisplay').innerText = event.detail.formattedTime;
});
</script>This approach adheres to Polymer's best practices by:
|
||
| }); | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,106 @@ | ||
| /*- | ||
| * #%L | ||
| * Template Add-on | ||
| * %% | ||
| * Copyright (C) 2024 Flowing Code | ||
| * %% | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| * #L% | ||
| */ | ||
|
|
||
| package com.flowingcode.addons.simpletimer.integration; | ||
|
|
||
| import com.vaadin.testbench.ScreenshotOnFailureRule; | ||
| import com.vaadin.testbench.TestBench; | ||
| import com.vaadin.testbench.parallel.ParallelTest; | ||
| import io.github.bonigarcia.wdm.WebDriverManager; | ||
| import org.junit.Before; | ||
| import org.junit.BeforeClass; | ||
| import org.junit.Rule; | ||
| import org.openqa.selenium.chrome.ChromeDriver; | ||
|
|
||
| /** | ||
| * Base class for ITs | ||
| * | ||
| * <p>The tests use Chrome driver (see pom.xml for integration-tests profile) to run integration | ||
| * tests on a headless Chrome. If a property {@code test.use .hub} is set to true, {@code | ||
| * AbstractViewTest} will assume that the TestBench test is running in a CI environment. In order to | ||
| * keep the this class light, it makes certain assumptions about the CI environment (such as | ||
| * available environment variables). It is not advisable to use this class as a base class for you | ||
| * own TestBench tests. | ||
| * | ||
| * <p>To learn more about TestBench, visit <a | ||
| * href="https://vaadin.com/docs/v10/testbench/testbench-overview.html">Vaadin TestBench</a>. | ||
| */ | ||
| public abstract class AbstractViewTest extends ParallelTest { | ||
| private static final int SERVER_PORT = 8080; | ||
|
|
||
| private final String route; | ||
|
|
||
| @Rule public ScreenshotOnFailureRule rule = new ScreenshotOnFailureRule(this, true); | ||
|
|
||
| public AbstractViewTest() { | ||
| this(""); | ||
| } | ||
|
|
||
| protected AbstractViewTest(String route) { | ||
| this.route = route; | ||
| } | ||
|
|
||
| @BeforeClass | ||
| public static void setupClass() { | ||
| WebDriverManager.chromedriver().setup(); | ||
| } | ||
|
|
||
| @Override | ||
| @Before | ||
| public void setup() throws Exception { | ||
| if (isUsingHub()) { | ||
| super.setup(); | ||
| } else { | ||
| setDriver(TestBench.createDriver(new ChromeDriver())); | ||
|
There was a problem hiding this comment. Ensure ChromeDriver runs in headless mode as per documentation. The Javadoc mentions that tests run on headless Chrome, but the current Apply this diff to update the imports and driver initialization: +import org.openqa.selenium.chrome.ChromeOptions;
...
if (isUsingHub()) {
super.setup();
} else {
+ ChromeOptions options = new ChromeOptions();
+ options.addArguments("--headless");
- setDriver(TestBench.createDriver(new ChromeDriver()));
+ setDriver(TestBench.createDriver(new ChromeDriver(options)));
}
|
||
| } | ||
| getDriver().get(getURL(route)); | ||
| } | ||
|
|
||
| /** | ||
| * Returns deployment host name concatenated with route. | ||
| * | ||
| * @return URL to route | ||
| */ | ||
| private static String getURL(String route) { | ||
| return String.format("http://%s:%d/%s", getDeploymentHostname(), SERVER_PORT, route); | ||
| } | ||
|
|
||
| /** Property set to true when running on a test hub. */ | ||
| private static final String USE_HUB_PROPERTY = "test.use.hub"; | ||
|
|
||
| /** | ||
| * Returns whether we are using a test hub. This means that the starter is running tests in | ||
| * Vaadin's CI environment, and uses TestBench to connect to the testing hub. | ||
| * | ||
| * @return whether we are using a test hub | ||
| */ | ||
| private static boolean isUsingHub() { | ||
| return Boolean.TRUE.toString().equals(System.getProperty(USE_HUB_PROPERTY)); | ||
| } | ||
|
|
||
| /** | ||
| * If running on CI, get the host name from environment variable HOSTNAME | ||
| * | ||
| * @return the host name | ||
| */ | ||
| private static String getDeploymentHostname() { | ||
| return isUsingHub() ? System.getenv("HOSTNAME") : "localhost"; | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| package com.flowingcode.addons.simpletimer.integration; | ||
|
|
||
| public interface IntegrationCallables { | ||
|
|
||
| void setStartTime(Integer startTime); | ||
|
|
||
| void setEndTime(Integer endTime); | ||
|
|
||
| void start(); | ||
|
|
||
| void pause(); | ||
|
|
||
| void reset(); | ||
|
|
||
| boolean isRunning(); | ||
|
|
||
| void openDialog(); | ||
|
|
||
| void closeDialog(); | ||
| // BigDecimal getCurrentTime(); | ||
| // | ||
| // CompletableFuture<BigDecimal> getCurrentTimeAsync(); | ||
| // | ||
| // Registration addCurrentTimeChangeListener(PropertyChangeListener listener, long period, | ||
| // TimeUnit periodUnit); | ||
| // | ||
| // Registration addTimerEndEvent(ComponentEventListener<TimerEndedEvent> listener); | ||
|
Comment on lines
+20
to
+27
There was a problem hiding this comment. Address commented-out method declarations. There are several commented-out method declarations at the end of the interface. These methods seem to provide additional functionality for time retrieval and event handling, which could be valuable for timer operations. However, leaving commented-out code in production can lead to confusion and maintenance issues. Consider one of the following actions:
Example of option 1: /**
* Gets the current time of the timer.
* @return the current time as a BigDecimal
* @throws UnsupportedOperationException if not yet implemented
*/
BigDecimal getCurrentTime();
// ... similar for other methods |
||
|
|
||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider removing
/webpack.config.jsfrom .gitignoreIt's generally not recommended to ignore
webpack.config.js. This file contains important configuration for your project's build process and should be version controlled to ensure consistency across different environments and developers.Consider removing this line from .gitignore:
-/webpack.config.js📝 Committable suggestion