feat(frontend): add toggle to remove line and page numbers from transliterations and translations #578

saihaj · 2020-06-04T00:04:26Z

Summary of PR

Using stripEndings() adds toggle that allows user to hide the line endings for translations and transliterations in the following settings tabs:

Display
Overlay
Search

Tests for unexpected behavior

Tested on browser and electron and works!
Passing null vals doesn't make it crash
Don't stripEndings for Sirlekh

Unhide Line Endings

Hide Line Endings:

Time spent on PR

6.5 hours

Linked issues

Fix #508

Reviewers

@Harjot1Singh @bhajneet

Note: Don't forget npm install since gurmukhi-util packages was updated.

…ings feature

Harjot1Singh · 2020-06-06T01:58:13Z

Note: feat(frontend/controller, overlay, presenter) is not correct, you'd need to use the full scope for each.

On a side note, I don't think it's actually correct to use the comma notation, even though I have, but instead use frontend solely as the scope

saihaj · 2020-06-06T02:00:11Z

Note: feat(frontend/controller, overlay, presenter) is not correct, you'd need to use the full scope for each.

On a side note, I don't think it's actually correct to use the comma notation, even though I have, but instead use frontend solely as the scope

So I can either do feat(frontend) or 3 different commits feat(frontend/controller) and etc...?

app/frontend/src/lib/options.js

Harjot1Singh

Generally, not sure I'm a fan of the parameter-passing, it's very similar to prop-drilling. Perhaps a utility function and/or usage of stripEndings directly would be a better approach? What if we wanted to add another option in the future? Would we keep extending the parameter list?

The approach otherwise is good.

saihaj · 2020-06-06T02:41:11Z

Generally, not sure I'm a fan of the parameter-passing, it's very similar to prop-drilling. Perhaps a utility function and/or usage of stripEndings directly would be a better approach? What if we wanted to add another option in the future? Would we keep extending the parameter list?

The approach otherwise is good.

It is possible to do use stripEndings before rendering but that is much more work and complicates things up. +1 for extending the list but we know the downside (the mess that we need to take care of). So we need a different way that will allow us to keep these functions higher up such that we don't have to deal with long parameter lists.

Harjot1Singh · 2020-06-14T11:53:39Z

Generally, not sure I'm a fan of the parameter-passing, it's very similar to prop-drilling. Perhaps a utility function and/or usage of stripEndings directly would be a better approach? What if we wanted to add another option in the future? Would we keep extending the parameter list?
The approach otherwise is good.

It is possible to do use stripEndings before rendering but that is much more work and complicates things up. +1 for extending the list but we know the downside (the mess that we need to take care of). So we need a different way that will allow us to keep these functions higher up such that we don't have to deal with long parameter lists.

Are you proposing a different solution, or did you mean you prefer extending the list? My suggestion is to use or write something like lodash's mapValues, so the code would look like mapValues( transliterations, stripEndings ), which is readable and clean.

saihaj · 2020-06-14T13:14:12Z

Generally, not sure I'm a fan of the parameter-passing, it's very similar to prop-drilling. Perhaps a utility function and/or usage of stripEndings directly would be a better approach? What if we wanted to add another option in the future? Would we keep extending the parameter list?

The approach otherwise is good.

It is possible to do use stripEndings before rendering but that is much more work and complicates things up. +1 for extending the list but we know the downside (the mess that we need to take care of). So we need a different way that will allow us to keep these functions higher up such that we don't have to deal with long parameter lists.

Are you proposing a different solution, or did you mean you prefer extending the list? My suggestion is to use or write something like lodash's mapValues, so the code would look like mapValues( transliterations, stripEndings ), which is readable and clean.

I preferred extending list, but something else that can make it readable will be amazing and I think I can change it to use the mapValues

saihaj · 2020-06-15T16:28:31Z

@Harjot1Singh I think we can add mapValues in future when it is extended. Right now there are only 2 things in the hooks so should be fine

All the customizaions will run through a function which can be expanded in future to include more transformations.

app/frontend/src/lib/utils.js

app/frontend/src/Controller/Search.js

app/frontend/src/Overlay/index.js

app/frontend/src/lib/utils.js

app/settings.default.json

app/frontend/src/lib/utils.js

app/frontend/src/Presenter/Display.js

saihaj · 2020-06-17T16:33:19Z

Should be good to merge now!

bhajneet · 2020-06-17T16:42:37Z

Option name: Line Ending
Value: False
Result: Shows line endings

???

saihaj · 2020-06-17T16:43:25Z

Can you give screenshot

bhajneet · 2020-06-17T16:43:52Z

Just going off the screenshots provided in PR -- was this updated/changed?

saihaj · 2020-06-17T16:45:08Z

nope

saihaj · 2020-06-17T16:47:45Z

Just going off the screenshots provided in PR

Which one ?

saihaj · 2020-06-17T17:23:02Z

Updated screenshots

Harjot1Singh · 2020-06-18T17:20:35Z

app/frontend/src/Overlay/index.js

@@ -23,18 +25,26 @@ const Overlay = () => {
  } } = globalSettings || {}

  const [ line ] = useCurrentLine()
+  const { typeId } = line || ''


Why the || '

because I was getting undefined errors so added it as a safety check

Deal with this inside customiseLine, since you presumably are doing this in multiple places

app/frontend/src/lib/consts.js

app/frontend/src/lib/utils.js

Harjot1Singh · 2020-06-23T18:48:06Z

app/frontend/src/Overlay/index.js

@@ -23,18 +25,26 @@ const Overlay = () => {
  } } = globalSettings || {}

  const [ line ] = useCurrentLine()
+  const { typeId } = line || ''


Deal with this inside customiseLine, since you presumably are doing this in multiple places

saihaj · 2020-06-23T19:22:30Z

the problem I can't handle there is this ->

Harjot1Singh · 2020-06-24T16:02:31Z

the problem I can't handle there is this ->

If you move the typeId || '' inside customiseLine, does this not solve that?

saihaj · 2020-06-24T16:22:08Z

the problem I can't handle there is this ->

If you move the typeId || '' inside customiseLine, does this not solve that?

I have to destructure typeId from line and when line is undefined I get the TypeError

Harjot1Singh · 2020-06-28T14:22:13Z

the problem I can't handle there is this ->

If you move the typeId || '' inside customiseLine, does this not solve that?

I have to destructure typeId from line and when line is undefined I get the TypeError

In this situation, it's ok to do the equivalent of line || {}, and then in customiseLine, typeId || ''. That should handle the TypeError

saihaj · 2020-06-28T15:45:37Z

Doing line || {} is sufficient

saihaj added 5 commits June 3, 2020 18:44

build: update gurmukhi util

42d50ab

build: sync package-lock

00e3706

feat(frontend/settings): add line ending toggle

23073f1

feat(frontend): use gurmukhi utils to strip line endings

fdff739

feat(frontend/controller, overlay, presenter): integrate the line end…

4b62e01

…ings feature

saihaj requested a review from Harjot1Singh June 4, 2020 00:04

bhajneet mentioned this pull request Jun 4, 2020

Line endings with non latin/gurmukhi numbers shabados/gurmukhi-utils#158

Closed

saihaj added Status: Hold and removed Status: Hold labels Jun 5, 2020

Harjot1Singh reviewed Jun 6, 2020

View reviewed changes

app/frontend/src/lib/options.js Show resolved Hide resolved

Harjot1Singh requested changes Jun 6, 2020

View reviewed changes

fix(backend): add default value to server settings

ea6f2bb

saihaj requested a review from Harjot1Singh June 8, 2020 01:38

saihaj assigned Harjot1Singh Jun 8, 2020

refactor(frontend): use a util to handle all customizations

a2ba1ff

All the customizaions will run through a function which can be expanded in future to include more transformations.

saihaj force-pushed the saihaj/issue508 branch from 8c5a1e2 to a2ba1ff Compare June 16, 2020 18:46

Harjot1Singh requested changes Jun 16, 2020

View reviewed changes

refactor: modify customiseLine

bf6228b

saihaj requested a review from Harjot1Singh June 16, 2020 23:40

refactor: add a new line

a73cf78

Harjot1Singh requested changes Jun 17, 2020

View reviewed changes

app/frontend/src/lib/utils.js Outdated Show resolved Hide resolved

app/frontend/src/lib/utils.js Outdated Show resolved Hide resolved

app/frontend/src/Presenter/Display.js Outdated Show resolved Hide resolved

saihaj added 2 commits June 17, 2020 11:17

refcator: use lodash instead of custom modified fn

26b2a02

refactor: destructure

75f5971

saihaj requested a review from Harjot1Singh June 17, 2020 16:24

fix: error check for null values

3b16280

refactor(frontend/settings): rename line ending setting

f2c017a

saihaj marked this pull request as draft June 17, 2020 17:23

fix(frontend): do not strip endings for sirlekh

ac0fc33

saihaj marked this pull request as ready for review June 17, 2020 18:52

Harjot1Singh requested changes Jun 18, 2020

View reviewed changes

saihaj added 2 commits June 18, 2020 12:29

refactor(frontend): change name for export

c2cd021

refactor(frontend): typeId is globally accessible

33cd089

saihaj requested a review from Harjot1Singh June 18, 2020 17:30

Harjot1Singh requested changes Jun 23, 2020

View reviewed changes

saihaj requested a review from Harjot1Singh June 27, 2020 02:51

refactor: pass in empty object to fix TypeError

c4b5083

Harjot1Singh approved these changes Jun 28, 2020

View reviewed changes

Harjot1Singh changed the title ~~feat(frontend): ability to remove line and page numbers (translit and translations)~~ feat(frontend): add toggle to remove line and page numbers from transliterations and translations Jun 28, 2020

Harjot1Singh merged commit ec21bbb into shabados:dev Jun 28, 2020

saihaj deleted the saihaj/issue508 branch June 28, 2020 18:26

saihaj restored the saihaj/issue508 branch June 28, 2020 18:31

saihaj deleted the saihaj/issue508 branch June 28, 2020 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(frontend): add toggle to remove line and page numbers from transliterations and translations #578

feat(frontend): add toggle to remove line and page numbers from transliterations and translations #578

saihaj commented Jun 4, 2020 •

edited

Loading

Harjot1Singh commented Jun 6, 2020

saihaj commented Jun 6, 2020

Harjot1Singh left a comment

saihaj commented Jun 6, 2020

Harjot1Singh commented Jun 14, 2020

saihaj commented Jun 14, 2020

saihaj commented Jun 15, 2020

saihaj commented Jun 17, 2020

bhajneet commented Jun 17, 2020

saihaj commented Jun 17, 2020

bhajneet commented Jun 17, 2020

saihaj commented Jun 17, 2020

saihaj commented Jun 17, 2020

saihaj commented Jun 17, 2020 •

edited

Loading

Harjot1Singh Jun 18, 2020

saihaj Jun 18, 2020

Harjot1Singh Jun 23, 2020

Harjot1Singh Jun 23, 2020

saihaj commented Jun 23, 2020 •

edited

Loading

Harjot1Singh commented Jun 24, 2020

saihaj commented Jun 24, 2020

Harjot1Singh commented Jun 28, 2020

saihaj commented Jun 28, 2020

feat(frontend): add toggle to remove line and page numbers from transliterations and translations #578

feat(frontend): add toggle to remove line and page numbers from transliterations and translations #578

Conversation

saihaj commented Jun 4, 2020 • edited Loading

Summary of PR

Tests for unexpected behavior

Time spent on PR

Linked issues

Reviewers

Harjot1Singh commented Jun 6, 2020

saihaj commented Jun 6, 2020

Harjot1Singh left a comment

Choose a reason for hiding this comment

saihaj commented Jun 6, 2020

Harjot1Singh commented Jun 14, 2020

saihaj commented Jun 14, 2020

saihaj commented Jun 15, 2020

saihaj commented Jun 17, 2020

bhajneet commented Jun 17, 2020

saihaj commented Jun 17, 2020

bhajneet commented Jun 17, 2020

saihaj commented Jun 17, 2020

saihaj commented Jun 17, 2020

saihaj commented Jun 17, 2020 • edited Loading

Harjot1Singh Jun 18, 2020

Choose a reason for hiding this comment

saihaj Jun 18, 2020

Choose a reason for hiding this comment

Harjot1Singh Jun 23, 2020

Choose a reason for hiding this comment

Harjot1Singh Jun 23, 2020

Choose a reason for hiding this comment

saihaj commented Jun 23, 2020 • edited Loading

Harjot1Singh commented Jun 24, 2020

saihaj commented Jun 24, 2020

Harjot1Singh commented Jun 28, 2020

saihaj commented Jun 28, 2020

saihaj commented Jun 4, 2020 •

edited

Loading

saihaj commented Jun 17, 2020 •

edited

Loading

saihaj commented Jun 23, 2020 •

edited

Loading