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

Translation helper improvement (added html parameter) #9696

Merged
merged 3 commits into from
Jun 5, 2021
Merged

Translation helper improvement (added html parameter) #9696

merged 3 commits into from
Jun 5, 2021

Conversation

imajit
Copy link
Contributor

@imajit imajit commented May 30, 2021

Part of #9686
Fixes #7798 #9494
As the translation function was called inside the HTML tag, there were rendering issues; I fixed it by adding an HTML = true optional parameter in the function, now we can pass html=false in cases where the function is called inside the HTML tag.
Current UI
Screenshot from 2021-05-30 17-22-52

After changes
Screenshot from 2021-05-30 17-23-32

@imajit imajit requested review from a team as code owners May 30, 2021 12:55
@gitpod-io
Copy link

gitpod-io bot commented May 30, 2021

codeclimate[bot]
codeclimate bot reviewed May 30, 2021
View reviewed changes
app/helpers/application_helper.rb Outdated
@@ -154,12 +154,12 @@ def current_page?(page)
'is-active' if request.path == page
end

def translation(key, options = {})
def translation(key, options = {},html = true)
Copy link

Choose a reason for hiding this comment

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

Space missing after comma.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Silly me

Copy link
Member

Choose a reason for hiding this comment

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

Don't take codeclimate bot /too/ seriously, it needs to learn to be friendlier! But thank you!

@codecov
Copy link

codecov bot commented May 30, 2021 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (main@7fe537a). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #9696   +/-   ##
=======================================
  Coverage        ?   81.89%           
=======================================
  Files           ?       98           
  Lines           ?     5928           
  Branches        ?        0           
=======================================
  Hits            ?     4855           
  Misses          ?     1073           
  Partials        ?        0

@jywarren
Copy link
Member

Ah it looks like perhaps the conflicts need to be resolved to trigger the tests?

@jywarren
Copy link
Member

Thanks, this is looking great so far!

@jywarren jywarren changed the title Translation function improvement Translation helper improvement (added html parameter) May 30, 2021
@codeclimate
Copy link

codeclimate bot commented May 30, 2021

Code Climate has analyzed commit 89935c4 and detected 0 issues on this pull request.

View more on Code Climate.

@RuthNjeri
Copy link
Contributor

This looks great @imajit 🎉 ...and thanks for replacing the t( with translation(

@RuthNjeri
Copy link
Contributor

The issue you have referenced #9696 has been closed, is there another main planning issue you can reference?

RuthNjeri
RuthNjeri approved these changes May 31, 2021
View reviewed changes
Copy link
Contributor

@RuthNjeri RuthNjeri left a comment

Choose a reason for hiding this comment

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

LGTM

@imajit
Copy link
Contributor Author

imajit commented May 31, 2021

The issue you have referenced #9696 has been closed, is there another main planning issue you can reference?

No, I guess it got closed when I tagged it in a different PR.
I reopened it

Tlazypanda
Tlazypanda approved these changes Jun 4, 2021
View reviewed changes
Copy link
Collaborator

@Tlazypanda Tlazypanda left a comment

Choose a reason for hiding this comment

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

@imajit LGTM 🎉

cesswairimu
cesswairimu approved these changes Jun 5, 2021
View reviewed changes
Copy link
Collaborator

@cesswairimu cesswairimu left a comment

Choose a reason for hiding this comment

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

Looking Great 🎉 thanks @imajit

@cesswairimu cesswairimu merged commit 16ec88b into publiclab:main Jun 5, 2021
@cesswairimu
Copy link
Collaborator

merged..lets confirm on stable after build.. thanks everyone

reginaalyssa pushed a commit to reginaalyssa/plots2 that referenced this pull request Oct 16, 2021
@imajit @reginaalyssa
Translation helper improvement (added html parameter) (publiclab#9696)
146e36b 
* Changed translation function

* Adding space

Co-authored-by: imajit <[email protected]>
billymoroney1 pushed a commit to billymoroney1/plots2 that referenced this pull request Dec 28, 2021
@imajit @billymoroney1
Translation helper improvement (added html parameter) (publiclab#9696)
ebe271a 
* Changed translation function

* Adding space

Co-authored-by: imajit <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeclimate codeclimate[bot] codeclimate[bot] left review comments

@jywarren jywarren jywarren left review comments

@Tlazypanda Tlazypanda Tlazypanda approved these changes

@cesswairimu cesswairimu cesswairimu approved these changes

@RuthNjeri RuthNjeri RuthNjeri approved these changes

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

Successfully merging this pull request may close these issues.

Translation string replacement errors
5 participants
@imajit @jywarren @RuthNjeri @cesswairimu @Tlazypanda