-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improving the dev workflow: Setup browserstack, add necessary configuration files and tests for mobile #5340
Changes from 1 commit
59adedb
a785896
60a731c
dd93d40
0bff650
1c09375
404e8d4
f07b6bb
8b4bc4b
9c7d3b4
14f5577
16e0cd6
1ac8185
bfb72ea
0a6f6f5
ffb648d
008326b
a3c48a9
40eeb8b
a6015d1
4bf14cc
d406e0d
3835b02
88f0632
4093284
b79707b
4630487
3e4cebe
97e9bd1
8217df9
09a2950
d4bc754
18094e9
f816deb
91115ef
e4dec5d
791c4be
e84ee31
631e037
81adf35
f88408f
92d9be7
ffa8680
8846971
8c9596d
0552a47
ebc3585
7802dbe
28ffb55
adc48a6
6439520
db0dd1b
c7b854f
57825a9
169c0f6
24801e8
0ea9141
6aabbf7
32f3410
8936847
93a44b0
8fcfd9d
fb6c144
af360dc
63a44b9
01375d2
6e857b0
efb6d59
6e0bee7
7f25654
0311d78
9e99692
0967489
70f9c3c
8446790
cbc746c
11314e0
7042a76
8fb7d5f
43aff58
aef0569
978f7e7
8cb84b3
a9c1dd2
b2454b3
0d5b996
8d901de
909c7fd
f0b05d5
cd1c226
3398940
fe2f70c
03c11ce
b7ce549
9526b61
61164f2
37fc3a3
6a53974
b253c5a
89f31bb
b42cffd
99e4474
ba52b39
8096443
796ff4c
66f71b2
ad88702
4e14f53
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,7 +46,7 @@ var AdminPage = function(){ | |
'.protractor-test-reload-all-explorations-button' | ||
)); | ||
|
||
var reloadCollectionButton = element.all(by.css( | ||
var reloadCollectionButtons = element.all(by.css( | ||
'.protractor-test-reload-collection-button')); | ||
|
||
var explorationTitleElement = function(explorationElement) { | ||
|
@@ -62,7 +62,7 @@ var AdminPage = function(){ | |
}; | ||
|
||
this.reloadCollection = function(collectionId) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe have a config that's DEV_MODE and then in this file do:
since these are only relevant there. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I didn't think about this. How do we access this variable here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When --prod_env flag is used, the globals DEV_MODE variable is flipped. 2)The little "Dev Mode" tag at the right corner of every page also disappears. You can take advantage of these GUI changes accordingly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for this -- quite helpful :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I was thinking of just declaring it similarly to what you have with browser.isMobile. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this approach a bit better in the sense that we are actually checking for the dev mode? I tested it out and it seems to work fine although we can go the other way if you'd like :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mean @hoangviet1993's approach? Yep, I'm fine with either; I was just answering your earlier question about "how do we access this variable here". (Sorry for not being clear.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool, understood! Thanks :) |
||
reloadCollectionButton.get(collectionId).click(); | ||
reloadCollectionButtons.get(collectionId).click(); | ||
general.acceptAlert(); | ||
// Time is needed for the reloading to complete. | ||
waitFor.textToBePresentInElement( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,7 +44,7 @@ var LibraryPage = function(){ | |
var languageSelector = forms.MultiSelectEditor( | ||
element(by.css('.protractor-test-search-bar-language-selector')) | ||
); | ||
var searchInput = element.all( | ||
var searchInputs = element.all( | ||
by.css('.protractor-test-search-input')); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
// Returns a promise of all explorations with the given name. | ||
|
@@ -156,9 +156,9 @@ var LibraryPage = function(){ | |
// The second input element is visible when the library | ||
// page is rendered for mobile device. | ||
|
||
var desktopSearchInput = searchInput.first(); | ||
var desktopSearchInput = searchInputs.first(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might as well define these vars in the relative branches. Split the doc above and put it in each branch:
(say) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, thanks! |
||
// get function is a zero-based index. | ||
var mobileSearchInput = searchInput.get(1); | ||
var mobileSearchInput = searchInputs.get(1); | ||
if (browser.isMobile) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should not duplicate so much code with findCollection(). Have both methods delegate to an internal method for selectLibraryTile(tileName) perhaps. Actually the name is ambiguous. Looks like it's just entering input in search bar, does it hit enter? Maybe submitSearchQuery() would be better. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Created a separate function, |
||
mobileSearchInput.clear(); | ||
mobileSearchInput.sendKeys(explorationTitle); | ||
|
@@ -176,9 +176,9 @@ var LibraryPage = function(){ | |
// The second input element is visible when the library | ||
// page is rendered for mobile device. | ||
|
||
var desktopSearchInput = searchInput.first(); | ||
var desktopSearchInput = searchInputs.first(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, thanks! |
||
// get function is a zero-based index. | ||
var mobileSearchInput = searchInput.get(1); | ||
var mobileSearchInput = searchInputs.get(1); | ||
if (browser.isMobile) { | ||
mobileSearchInput.clear(); | ||
mobileSearchInput.sendKeys(collectionTitle); | ||
|
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.
reloadCollectionButtons
to reflect plurality here.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.
Done, thanks!