search

vaadin / board

Framework 8 version of vaadin-board
Other
8 stars 10 forks source link

Feature/sru 001 #40

Closed svenruppert closed 7 years ago

svenruppert commented 7 years ago

changed the const, that were used inside json. to be comp. to Selenium const.

This change is Reviewable

alvarezguille commented 7 years ago

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

a discussion (no related file): just a suggestion related to change in capitalization between "Internet Explorer" and BrowserType.IE

should we force lowercase when we read it in json file to avoid problems? opinions @rogozinds @svenruppert ?

Comments from Reviewable

svenruppert commented 7 years ago

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

a discussion (no related file):

Previously, alvarezguille (Guille) wrote…
just a suggestion related to change in capitalization between `"Internet Explorer"` and `BrowserType.IE` should we force lowercase when we read it in json file to avoid problems? opinions @rogozinds @svenruppert ?

in source code i would use the BrowserType const. in Functions I just adding the lowercase as additional step as long as we have no schema for json

Comments from Reviewable

alvarezguille commented 7 years ago

a discussion (no related file):

Previously, svenruppert (Sven Ruppert) wrote…
in source code i would use the BrowserType const. in Functions I just adding the lowercase as additional step as long as we have no schema for json

I agree, that would prevent some surprises

Comments from Reviewable

rogozinds commented 7 years ago

Reviewed 3 of 3 files at r1. Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

rogozinds commented 7 years ago
:lgtm:

Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

alvarezguille commented 7 years ago

@svenruppert In the future if possible please add a descriptive title to the PR. As we are squashing and merging and the title is most probably going to end up as the commit message so it should be more understandable than "Feature/sru 001". Now I changed it when merging fab44b3.