Skip to content
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

Modularize mobile timeline wheel #651

Merged
merged 4 commits into from
Jan 8, 2018
Merged

Conversation

localjo
Copy link
Contributor

@localjo localjo commented Jan 2, 2018

Based on update-build-scripts (#647)

Modularizes mobile timeline wheel. Had to revert mobiscroll to 2.6.x and put it back in the /web/ext directory. The newer version of mobiscroll requires a license to access full functionality, so we need to stick with the old version until we can replace this dependency.

This PR also fixes various mobile bugs.

@ghost ghost assigned localjo Jan 2, 2018
@ghost ghost added the under development label Jan 2, 2018
@Benjaki2 Benjaki2 mentioned this pull request Jan 2, 2018
58 tasks
@localjo localjo force-pushed the modularize-mobile-timeline branch from 56f5501 to 5bc99f0 Compare January 3, 2018 20:05
Copy link
Contributor

@ZachTRice ZachTRice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While using the timewheel I am seeing Uncaught TypeError: Cannot read property 'classList' of null

I'm having trouble finding a consistent way to reproduce the error.

It seems I see it when I open the timeline wheel, drag the viewport window, then open the timewheel again and change the dates. It's strange too because the wheel seems to glitch out and allow for the left/right keyboard arrows to control the date and at this point then every time I use the arrows to change the date, I see the arrow.

I am also able to see this happen via browserstack (S8 Chrome). I open timeline wheel, then change the orientation, then open timewheel again and change the data. The same glitch with the arrows happens and I see the console arrows.

Note: The most consistent way to reproduce the error seems to be to use browserstack, open timewheel, change orientation, use left/right arrow keys to change the date.

I tried reproducing this on the main app to make sure it wasn't happening there as well but I wasn't able to reproduce it.

@localjo localjo requested a review from Benjaki2 January 5, 2018 17:09
@localjo localjo force-pushed the modularize-mobile-timeline branch from 9e152f9 to 328bc83 Compare January 5, 2018 17:17
@localjo localjo force-pushed the modularize-mobile-timeline branch from 328bc83 to 294c77f Compare January 5, 2018 17:29
The newer version of mobiscroll requires a license to access full functionality, so we need to stick with the old version until we can replace this dependency
@localjo localjo force-pushed the modularize-mobile-timeline branch from 294c77f to c4dd053 Compare January 5, 2018 17:33
@localjo
Copy link
Contributor Author

localjo commented Jan 5, 2018

I can't reproduce that error.

@ZachTRice Are you seeing the timeline wheel appear on desktop? I wouldn't think it works on desktop. And what arrows are you using to change the date on mobile? I'm not seeing any arrows.

@ZachTRice
Copy link
Contributor

Yeah, the timewheel appears on the responsive desktop view when you click the date in the corner.

@ZachTRice
Copy link
Contributor

Note: I am only able to reproduce this when the source is minified.

Copy link
Collaborator

@Benjaki2 Benjaki2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -89,3 +89,5 @@ export function dateWheels(models, config) {
init();
return self;
};

export default dateWheels;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@localjo what's the reason for making this a default export instead of exporting the function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dateWheels is a function. These two are more or less equivalent;

export default function(){};

And

var dateWheels = function(){};
export default dateWheels;

I don't remember exactly why I did it this way here. Probably just to be clearer by giving a name to what I was exporting.

@localjo localjo merged commit e8bb844 into module-loaders Jan 8, 2018
@ghost ghost removed the under development label Jan 8, 2018
@localjo localjo deleted the modularize-mobile-timeline branch January 9, 2018 16:13
@localjo localjo added this to the Module Loaders Release (v2.0) milestone Jan 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants