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.

Sign up for GitHub

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

Jump to bottom

Feature/300 musical notes #310

Merged
merged 16 commits into from
Sep 3, 2021
Merged

Feature/300 musical notes #310

merged 16 commits into from
Sep 3, 2021

Conversation

zuevval
Copy link
Member

@zuevval zuevval commented Aug 29, 2021

closes #300

I've added notes, and other symbols, too.

For notes, I've implemented a special mode in Browser where you can select the note duration and play the tune.

screenshot screenshot

@zuevval zuevval added enhancement New feature or request lessons Lessons section improvement practice Practice section improvement labels Aug 29, 2021
@zuevval zuevval added this to the 1.2.2 or 1.3.0 milestone Aug 29, 2021
@zuevval zuevval linked an issue Aug 29, 2021 that may be closed by this pull request
Add musical symbols to materials #300
Closed
3 tasks
winter-yuki
winter-yuki reviewed Aug 30, 2021
View reviewed changes
Copy link
Member

@winter-yuki winter-yuki left a comment

Choose a reason for hiding this comment

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

Great work!

app/src/main/AndroidManifest.xml Show resolved Hide resolved
...rc/main/java/com/github/braillesystems/learnbraille/ui/screens/browser/MarkerViewFragment.kt Outdated Show resolved Hide resolved
...rc/main/java/com/github/braillesystems/learnbraille/ui/screens/browser/MarkerViewFragment.kt Outdated Show resolved Hide resolved
...rc/main/java/com/github/braillesystems/learnbraille/ui/screens/browser/MarkerViewFragment.kt Outdated Show resolved Hide resolved
...rc/main/java/com/github/braillesystems/learnbraille/ui/screens/browser/MarkerViewFragment.kt Outdated Show resolved Hide resolved
app/src/main/java/com/github/braillesystems/learnbraille/ui/screens/practice/CardFragment.kt Outdated Show resolved Hide resolved
winter-yuki
winter-yuki requested changes Aug 31, 2021
View reviewed changes
Copy link
Member

@winter-yuki winter-yuki left a comment

Choose a reason for hiding this comment

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

I've found a lack of the database migration.
Thank you for the following my recommendations!

...rc/main/java/com/github/braillesystems/learnbraille/ui/screens/browser/MarkerViewFragment.kt Outdated
text = noteDurationTemplate.format(text, getString(noteDuration.titleStrId))
durationButton.visibility = View.VISIBLE
durationButton.setOnClickListener {
chooseNoteDurationDialog(brailleDots = dots, block = {
Copy link
Member

@winter-yuki winter-yuki Aug 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

When a block is the last function param is a lambda, it can be moved outside the parentheses:

chooseNoteDurationDialog(brailleDots = dots) {
   ...
}

Copy link
Member Author

Choose a reason for hiding this comment

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

done

app/src/main/java/com/github/braillesystems/learnbraille/res/Materials.kt
@@ -34,7 +34,32 @@ enum class MarkerType {
LatinSmall,
BoldFont,
ItalicFont,
NumberSign
NumberSign,
Copy link
Member

Choose a reason for hiding this comment

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

You should write a simple database migration to update material data for users

Copy link
Member Author

Choose a reason for hiding this comment

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

May I write it later? The course will change before the release. Or it will be a separate migration?

Copy link
Member

Choose a reason for hiding this comment

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

Okey, we just should not forget about it

app/src/main/java/com/github/braillesystems/learnbraille/ui/screens/practice/CardFragment.kt Outdated
@@ -104,7 +105,13 @@ class CardFragment : AbstractFragmentWithHelp(R.string.practice_help) {
is MarkerSymbol -> {
binding.letter.visibility = View.GONE
binding.markerDescription.visibility = View.VISIBLE
binding.markerDescription.text = inputMarkerPrintRules[it.type]
val baseDescription = inputMarkerPrintRules[it.type]
binding.markerDescription.text = when (it.type) {
Copy link
Member

Choose a reason for hiding this comment

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

It is better to write if expression if when has only two branches

Copy link
Member Author

Choose a reason for hiding this comment

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

done

...rc/main/java/com/github/braillesystems/learnbraille/ui/screens/browser/MarkerViewFragment.kt Outdated
) {
val input = RadioGroup(context)
var noteDuration = NoteDuration.EIGHTH
NoteDuration.values().map { duration ->
Copy link
Member

Choose a reason for hiding this comment

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

map return value is not used here, so it is better to use forEach extension function

Copy link
Member Author

Choose a reason for hiding this comment

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

changed this, thanks

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
...rc/main/java/com/github/braillesystems/learnbraille/ui/screens/browser/MarkerViewFragment.kt Outdated
})
}

AlertDialog.Builder(context)
Copy link
Member

Choose a reason for hiding this comment

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

Is Ok button unnecessary here? It looks like it is redundant and the only choice is enough

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right. I've removed the button and made some other small improvements. Now it looks like this

screenshot

@zuevval zuevval mentioned this pull request Sep 2, 2021
Open
@zuevval zuevval requested a review from winter-yuki September 2, 2021 19:01
winter-yuki
winter-yuki approved these changes Sep 3, 2021
View reviewed changes
Copy link
Member

@winter-yuki winter-yuki left a comment

Choose a reason for hiding this comment

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

Looks good to me

app/src/main/java/com/github/braillesystems/learnbraille/res/Materials.kt
@@ -34,7 +34,32 @@ enum class MarkerType {
LatinSmall,
BoldFont,
ItalicFont,
NumberSign
NumberSign,
Copy link
Member

Choose a reason for hiding this comment

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

Okey, we just should not forget about it

@winter-yuki winter-yuki merged commit 2c17448 into dev Sep 3, 2021
@winter-yuki winter-yuki deleted the feature/300-musical-notes branch September 3, 2021 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@winter-yuki winter-yuki winter-yuki approved these changes

Assignees

@winter-yuki winter-yuki

Labels
enhancement New feature or request lessons Lessons section improvement practice Practice section improvement
Projects
None yet
Milestone
1.2.2 or 1.3.0
Development

Successfully merging this pull request may close these issues.

Add musical symbols to materials
2 participants
@zuevval @winter-yuki