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

Format display numbers #17

Closed
wants to merge 32 commits into from
Closed

Format display numbers #17

wants to merge 32 commits into from

Conversation

yCobanoglu
Copy link
Member

No description provided.

@jlaasonen jlaasonen added the enhancement New feature or request label Apr 17, 2020
Copy link
Member

@jlaasonen jlaasonen left a comment

Choose a reason for hiding this comment

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

Please remove the spurious ruling changes.

src/fragmentarium/ui/display/Display.sass Outdated Show resolved Hide resolved
src/fragmentarium/ui/display/Display.sass Show resolved Hide resolved
src/fragmentarium/ui/display/Transliteration.tsx Outdated Show resolved Hide resolved
src/fragmentarium/ui/display/Transliteration.tsx Outdated Show resolved Hide resolved
src/fragmentarium/ui/display/Transliteration.tsx Outdated Show resolved Hide resolved
src/fragmentarium/ui/display/Transliteration.tsx Outdated Show resolved Hide resolved
src/fragmentarium/ui/display/Transliteration.tsx Outdated Show resolved Hide resolved
src/fragmentarium/ui/display/Transliteration.tsx Outdated Show resolved Hide resolved
Comment on lines 252 to 257
const rulingsToNumber: ReadonlyMap<string, number> = new Map([
['SINGLE', 1],
['DOUBLE', 2],
['TRIPLE', 3]
])

Copy link
Member

Choose a reason for hiding this comment

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

Why is this here?

src/fragmentarium/ui/display/Transliteration.tsx Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Apr 18, 2020

This pull request introduces 1 alert when merging 3b815d9 into 01821ac - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

src/fragmentarium/ui/display/LineAccumulator.tsx Outdated Show resolved Hide resolved
)
}

function isCloseEnclosure(token: Token): boolean {
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

return isEnclosure(token) && ['CENTER', 'RIGHT'].includes(token.side)
}

function isOpenEnclosure(token: Token): boolean {
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

This was referenced Apr 19, 2020
Closed
@yCobanoglu
Copy link
Member Author

Copy link
Member

@jlaasonen jlaasonen left a comment

Choose a reason for hiding this comment

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

Please, remove the code regarding the rulings.

src/fragmentarium/ui/display/LineAccumulator.tsx Outdated Show resolved Hide resolved
src/fragmentarium/ui/display/LineAccumulator.tsx Outdated Show resolved Hide resolved
content
}: {
content: ReadonlyArray<Token>
}): any {
Copy link
Member

Choose a reason for hiding this comment

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

src/fragmentarium/ui/display/Transliteration.tsx Outdated Show resolved Hide resolved
src/fragmentarium/ui/display/Transliteration.tsx Outdated Show resolved Hide resolved
src/fragmentarium/ui/display/Transliteration.tsx Outdated Show resolved Hide resolved
src/fragmentarium/ui/display/Transliteration.tsx Outdated Show resolved Hide resolved
src/fragmentarium/ui/display/Transliteration.tsx Outdated Show resolved Hide resolved
)
}

function isCloseEnclosure(token: Token): boolean {
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

return isEnclosure(token) && ['CENTER', 'RIGHT'].includes(token.side)
}

function isOpenEnclosure(token: Token): boolean {
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

@codeclimate
Copy link

codeclimate bot commented Apr 20, 2020

Code Climate has analyzed commit d554f06 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 2

The test coverage on the diff in this pull request is 100.0% (80% is the threshold).

This pull request will bring the total coverage in the repository to 89.4% (0.1% change).

View more on Code Climate.

@lgtm-com
Copy link

lgtm-com bot commented Apr 20, 2020

This pull request introduces 2 alerts when merging 57dbb01 into 01821ac - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@jlaasonen
Copy link
Member

Moved to #20

@jlaasonen jlaasonen closed this Apr 21, 2020
@jlaasonen jlaasonen deleted the format_display_numbers branch April 21, 2020 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants