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

added settings for language control #101

Merged
merged 4 commits into from
Oct 21, 2019
Merged

added settings for language control #101

merged 4 commits into from
Oct 21, 2019

Conversation

sprakash57
Copy link
Contributor

@sprakash57 sprakash57 commented Oct 21, 2019 •
edited
Loading

PR Checklist

Please check all that apply to this PR using "x":

  • I have checked that this PR is not a duplicate of an existing PR (open, closed or merged)
  • I have checked that this PR does not introduce a breaking change
  • This PR introduces breaking changes and I have provided a detailed explanation below

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting)
  • Refactoring (no functional changes)
  • Documentation changes
  • Other - Please describe:

Fixes

Issue Number: #76 #92

What is the current behavior?

What is the new behavior?

Other information

  • Wrapped all strings inside the localization function.
  • Created option to set default locale ( language ) and control on context state.
  • Added messages.fr.json
  • Added some missing fields for Portuguese and German locale.

accesslint[bot]
accesslint bot suggested changes Oct 21, 2019
View reviewed changes
Copy link

@accesslint accesslint bot 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 accessibility issues in these changes.

src/components/toolbar/select.js Outdated Show resolved Hide resolved
src/components/toolbar/index.js Outdated Show resolved Hide resolved
@sprakash57 sprakash57 mentioned this pull request Oct 21, 2019
Open
4 tasks
btzr-io
btzr-io requested changes Oct 21, 2019
View reviewed changes
Copy link
Owner

@btzr-io btzr-io left a comment

Choose a reason for hiding this comment

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

String keys are confusing, can you rename it to its English equivalent, like this:

{
 "Previous page": "Previous page",
  "Next page": "Next page",
....
}

I'm using atom so there is tool to replace all strings in the project in case you need something like that.

src/components/toolbar/index.js Outdated Show resolved Hide resolved
accesslint[bot]
accesslint bot suggested changes Oct 21, 2019
View reviewed changes
Copy link

@accesslint accesslint bot 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 accessibility issues in these changes.

src/components/toolbar/select.js Outdated
<label style={{ marginTop: '5px', marginRight: '5px' }}>
<Icon path={icon} size={'22px'} className={'villain-icon'} />
</label>
<select onClick={this.handleClick}>
Copy link

Choose a reason for hiding this comment

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

Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added label as icon.

Copy link
Owner

Choose a reason for hiding this comment

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

it needs a for attribute or you can just wrap the select component inside the label.

I think better to just wrap it ^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrapped under label tag. Also i have capitalized locale code for better visibility.

btzr-io
btzr-io approved these changes Oct 21, 2019
View reviewed changes
Copy link
Owner

@btzr-io btzr-io left a comment

Choose a reason for hiding this comment

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

Just a small change on the select component: #101 (comment)

accesslint[bot]
accesslint bot approved these changes Oct 21, 2019
View reviewed changes
Copy link

@accesslint accesslint bot left a comment

Choose a reason for hiding this comment

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

👏 You fixed the issue(s)! Great work.

@btzr-io
Copy link
Owner

btzr-io commented Oct 21, 2019

@sprakash57 Nice great contribution, probably I'll end up removing the select component from the toolbar itself, because there is no more space on the toolbar. see: #13 #34

@btzr-io
Copy link
Owner

btzr-io commented Oct 21, 2019 •
edited
Loading

@sprakash57 Does the auto language detection works ?
Will there be any error if the current user language is not defined / exist yet on the localize folder ?
Does it have a fallback to english ?

@btzr-io
Copy link
Owner

btzr-io commented Oct 21, 2019

The first language is considered the default one, so if a translation is missing for the selected language, the default one is shown and a line is written to the log as a reminder.

btzr-io
btzr-io requested changes Oct 21, 2019
View reviewed changes
src/components/toolbar/zoom.js
@@ -75,7 +75,7 @@ class ZoomControls extends Component {
<Button
type={'icon'}
icon={mdiMinus}
tooltip={messages['zoom.out']}
tooltip={messages['Zoom out']}
Copy link
Owner

Choose a reason for hiding this comment

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

this should use the new Localize implementation

src/components/toolbar/zoom.js
@@ -66,7 +66,7 @@ class ZoomControls extends Component {
<React.Fragment>
<Button
type={'icon'}
tooltip={messages['zoom.in']}
tooltip={messages['Zoom in']}
Copy link
Owner

Choose a reason for hiding this comment

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

this should use the new Localize implementation

src/context.js Outdated
@@ -23,6 +24,7 @@ const defaultState = {
fullscreen: false,
showControls: false,
autoHideControls: false,
changeLang: Localization.getLanguage(),
Copy link
Owner

Choose a reason for hiding this comment

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

this should use localization.getInterfaceLanguage() also rename to language

src/context.js
@@ -68,6 +70,11 @@ export class ReaderProvider extends Component {
}))
}

toggleLang = lang => {
Copy link
Owner

Choose a reason for hiding this comment

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

rename to toggleLanguage for readability

@btzr-io
Copy link
Owner

btzr-io commented Oct 21, 2019

@sprakash57 Can you add language auto-detection ?

@btzr-io
Copy link
Owner

btzr-io commented Oct 21, 2019

can you fix the conflicts ?

@btzr-io btzr-io merged commit f2a5975 into btzr-io:master Oct 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@btzr-io btzr-io btzr-io requested changes

@accesslint accesslint[bot] accesslint[bot] approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sprakash57 @btzr-io