Closed saihaj closed 4 years ago
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
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...?
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.
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.
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 likemapValues( 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
@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
If null
is passed to stripEndings
then it crashes. Do I do that sort of checking in here or should Gurmukhi-util(v3.0.0) handle that?
Update: handling it in here
Should be good to merge now!
Option name: Line Ending Value: False Result: Shows line endings
???
Can you give screenshot
Just going off the screenshots provided in PR -- was this updated/changed?
nope
Just going off the screenshots provided in PR
Which one ?
Updated screenshots
the problem I can't handle there is this ->
the problem I can't handle there is this ->
![]()
If you move the typeId || ''
inside customiseLine
, does this not solve that?
the problem I can't handle there is this ->
![]()
If you move the
typeId || ''
insidecustomiseLine
, does this not solve that?
I have to destructure typeId
from line
and when line
is undefined I get the TypeError
the problem I can't handle there is this ->
![]()
If you move the
typeId || ''
insidecustomiseLine
, does this not solve that?I have to destructure
typeId
fromline
and whenline
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
Doing line || {}
is sufficient
Summary of PR
Using
stripEndings()
adds toggle that allows user to hide the line endings for translations and transliterations in the following settings tabs:Tests for unexpected behavior
stripEndings
for SirlekhUnhide 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.