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

tutorial with localization added to stopwatch activity #636

Merged
merged 12 commits into from
Mar 7, 2020

Conversation

ksraj123
Copy link
Contributor

@ksraj123 ksraj123 commented Feb 22, 2020

fixes #596

  • Should display a welcome message to explain the activity (could reuse the help message describing the activity in Sugarizer activities List view)
  • Should explain usage of each button in the toolbar
  • Should explain usage of each element in the UI
  • Should be localized
  • Should respect the Sugarizer UI, as explained in tutorial

stopwatch tutorial

@Swayamsvk
Copy link

I don't think this fix supports other languages.

@ksraj123
Copy link
Contributor Author

ksraj123 commented Feb 22, 2020

Code for localization has been set up. Native Speakers will translate the messages.

@ksraj123 ksraj123 force-pushed the stopwatch-activity-tutorial branch 3 times, most recently from b1a038f to e7f7694 Compare February 23, 2020 12:22
@ksraj123
Copy link
Contributor Author

ksraj123 commented Feb 23, 2020

Added Scrolling support to the tutorial.
stopwatchscrolltutorial

Copy link
Owner

@llaske llaske left a comment

Choose a reason for hiding this comment

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

Nice start. See my comments below.

@ksraj123 ksraj123 force-pushed the stopwatch-activity-tutorial branch from e7f7694 to 208a327 Compare February 23, 2020 20:17
@llaske
Copy link
Owner

llaske commented Feb 23, 2020

That's fine but something is wrong regarding the font/style used by popup. See for example this comparison with another tutorial:

image

Plus, could you reorder items in steps array? The idea is too put at first the most important. So I guess it could be: start button, reinit, time laps, counter, remove button, add button, ...

@ksraj123
Copy link
Contributor Author

ksraj123 commented Feb 24, 2020

Issues fixed based on suggestions

  • Font issue in tutorial fixed. Unlike other activities, this one was using bootstrap already and bootstrap.min.css and bootstrap-tour-standalone.min.css had conflicting properties for some classes. I guess this was causing the issue.
  • Reordered tutorials popups.
  • Pop up for Add Stopwatch button made to appear on top if it has little space below it. The popup overlaps with the button when all stopwatches are removed if it is fixed to top so conditional logic is needed.

Before -
The pop up touching the bottom of the screen does not look alight. Problem specially noticable when the App Stopwatch button is out of view and needs to be scrolled to.
Screenshot from 2020-02-24 19-13-56

After Changes -
stoptut

@ksraj123 ksraj123 requested a review from llaske February 24, 2020 21:11
@abhishektanwar
Copy link
Contributor

@ksraj123 I believe the styling problem still persist in the popup .

@ksraj123
Copy link
Contributor Author

@abhishektanwar I think bootstrap was overriding the line-height of popup title as well, fixed that. Apart from that they look the same to my eyes.

compare

@llaske
Copy link
Owner

llaske commented Feb 27, 2020

Hmm I suggest to try to use bootstrap-tour.min.css instead of bootstrap-tour-standalone.min.css because the later already include bootstrap.min.css. It could the reason of the conflict.

Could you also solve conflict related to PR merged since your last commit?

@llaske llaske removed their request for review February 27, 2020 21:11
@ksraj123
Copy link
Contributor Author

ksraj123 commented Mar 4, 2020

@llaske suggested changes made and merge conflict resolved. Please review.
Apologies for the delay, had my university exams going on.

boostrap-tour-standalone.css and bootstrap-tour-standalone.js replaced with bootstrap-tour.css and bootstrap-tour.js respectively of the same version of bootstrap tour i.e. v10.2. The bootstrap-tour-standalone used in this project and others which have same styling included v3.1.0 of bootstrap but this project was already using v3.3.4 and this difference in versions of bootstrap is the root cause of all styling issues in tutorial of this activity. Now since bootstrap-tour.css does not have its own appropriate copy of bootstrap issues become apparent again.

Issue fixed by overriding concerned conflicting properties of bootstrap v3.3.4 with their v3.1.0 counterparts in activity.css file.

@ksraj123 ksraj123 force-pushed the stopwatch-activity-tutorial branch from c3a8902 to 74b091b Compare March 4, 2020 23:11
@ksraj123 ksraj123 requested a review from llaske March 6, 2020 09:36
@llaske
Copy link
Owner

llaske commented Mar 7, 2020

Nice. The UI issue is now fixed.

BTW the localization doesn't work: the Sugarizer language settings is not used.
It's probably related to the fact that it's not initialized in the activity. See here for more.

@llaske llaske removed their request for review March 7, 2020 14:44
@ksraj123
Copy link
Contributor Author

ksraj123 commented Mar 7, 2020

@llaske Suggested change made, Localisation initialised.
localization added to stop watch

@llaske llaske merged commit 79f64a8 into llaske:dev Mar 7, 2020
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.

4 participants