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

Feature/use known option #207

Closed
wants to merge 10 commits into from
Closed

Feature/use known option #207

wants to merge 10 commits into from

Conversation

winter-yuki
Copy link
Member

@winter-yuki winter-yuki commented Jul 9, 2020

Closes #203

Copy link
Member

@zuevval zuevval left a comment

Choose a reason for hiding this comment

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

I suppose it should be implemented like this 👇
photo5334853104442649977

@winter-yuki winter-yuki closed this Jul 9, 2020
Copy link
Member

@zuevval zuevval left a comment

Choose a reason for hiding this comment

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

There are three changes in this request:

  1. docs
  2. help margins
  3. practice options changed

The last I do not support for reasons described above (also: I plan to probably add more decks, e. g. english, spanish, french, deutsh, math, musical, chemical...; I plan to probably add a writing mode, where letters are mirrored, add an option to switch on/off higher probability of occurence of letters that user typed in incorrectly; this all seems to be more logical to control within a single screen, which is accessible in one click and icon is BIG and MEANINGFUL (who knows what's behind "other options"?)).

Let's raise a discussion? @braille-systems/learn-braille-team

helpItems.forEach { helpItem ->
val textView = TextView(context).apply {
setPaddingRelative(2, 0, 2, 0)
setPaddingRelative(30, 0, 20, 0)
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be right, thank you. At least popular readers on Android feature margins. Compare:

@winter-yuki
Copy link
Member Author

winter-yuki commented Jul 12, 2020

There are three changes in this request:

1. docs

2. help margins

3. practice options changed

The last I do not support for reasons described above (also: I plan to probably add more decks, e. g. english, spanish, french, deutsh, math, musical, chemical...; I plan to probably add a writing mode, where letters are mirrored, add an option to switch on/off higher probability of occurence of letters that user typed in incorrectly; this all seems to be more logical to control within a single screen, which is accessible in one click and icon is BIG and MEANINGFUL (who knows what's behind "other options"?)).

Let's raise a discussion? @braille-systems/learn-braille-team

This PR solves a particular problem. Not going to redesign something general here.

@zuevval
Copy link
Member

zuevval commented Jul 12, 2020

This PR solves a particular problem. Not going to redesign something general here.

It solves a problem in a strange fashion.
And, besides, brings a new problem: "change deck" menu is not accessible directly from practice

@zuevval
Copy link
Member

zuevval commented Jul 12, 2020

there are basically two types of features:

  1. Proof-of-concept. These needn't bee super-convenient, polished and reliable
  2. Usability improvement. Here we should do our best.
    This one is of the second type. So I'm going to try an alternative design myself.

@zuevval
Copy link
Member

zuevval commented Jul 12, 2020

here is my vision (though not fully functional, but design is ~ as I would like to see)

@kaustika
Copy link
Member

I prefer the way @winter-yuki implemented it tbh. Its easily accessed from where its needed the most - right from the practice with one click.
But maybe(?) it would be useful to have the same switch in general settings, thinking of it as a place where all the preferences can be set.

@zuevval
Copy link
Member

zuevval commented Jul 13, 2020

@kaustika thank you. But in my version these settings are accessible in one click, too. This menu pops up on “change decks” button click

@zuevval
Copy link
Member

zuevval commented Jul 14, 2020

I trust in good judgement and common sense of @kaustika and @winter-yuki so let's for now it be like this.

@winter-yuki what do you think about keeping the switch in general settings? I suppose it's a good idea, maybe you could do it?

@zuevval
Copy link
Member

zuevval commented Jul 14, 2020

@winter-yuki please, return the switch (or decide not to return) and let me know when you're done, I will change the help message accordingly and merge this request.

@zuevval
Copy link
Member

zuevval commented Jul 15, 2020

I will delete the corresponding branch.

But will save this code in my clone of this repo: https://github.com/zuevval/learn-braille/tree/feature/use-known-option

Just in case.

@zuevval zuevval deleted the feature/use-known-option branch July 15, 2020 09:59
@zuevval zuevval mentioned this pull request Aug 17, 2020
3 tasks
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.

None yet

3 participants