Skip to content

Use new localized context state and source in logbook#12742

Merged
zsarnett merged 11 commits intohome-assistant:devfrom
bdraco:logbook_localize
May 23, 2022
Merged

Use new localized context state and source in logbook#12742
zsarnett merged 11 commits intohome-assistant:devfrom
bdraco:logbook_localize

Conversation

@bdraco
Copy link
Copy Markdown
Member

@bdraco bdraco commented May 20, 2022

Does not need to wait for home-assistant/core#72333 since we can't rewrite history. (the source was only exposed on the original message and missing from the context message).

Proposed change

Fixes #12698 (except for the backend messages that come from integrations since we don't control thoose)

Screen Shot 2022-05-20 at 14 18 19

Screen Shot 2022-05-20 at 14 17 47

Screen Shot 2022-05-20 at 14 17 42

Screen Shot 2022-05-20 at 14 17 30

Screen Shot 2022-05-20 at 14 17 13

Screen Shot 2022-05-20 at 14 16 46

Screen Shot 2022-05-20 at 14 16 39

Screen Shot 2022-05-20 at 14 16 33

Screen Shot 2022-05-20 at 14 37 34

Screen Shot 2022-05-20 at 14 38 55

Screen Shot 2022-05-20 at 14 46 15

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue or discussion:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

- Refactor to remove more static english strings that come from the backend
@bdraco bdraco force-pushed the logbook_localize branch 2 times, most recently from c73d5c2 to e9f9da8 Compare May 22, 2022 05:42
@bdraco bdraco force-pushed the logbook_localize branch from e9f9da8 to 7a4f9ec Compare May 22, 2022 05:54
@bdraco bdraco changed the title Use new localized context state in logbook Use new localized context state and source in logbook May 22, 2022
@bdraco bdraco marked this pull request as ready for review May 22, 2022 21:59
@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented May 22, 2022

I moved localizeTriggerSource next to localizeStateMessage in data/logbook since it seemed like it was a better fit there.

@kukulich
Copy link
Copy Markdown
Contributor

I've really tried to understand the PR and I'm not sure if it fully fixes #12698 ...

It looks much much better (eg. ui.components.logbook.for is gone) but it still looks (based on the screenshots) that texts are concatenated together... It would be still difficult for translator to translate it in a way that make sense.

Something like this can be traslated more easier:

Light turned on
Triggered by automatition: Some automatition name
Triggered by state of: Some entity

@zsarnett
Copy link
Copy Markdown
Contributor

We can merge today most likely. Ill review this soon. Just need to fix the conflicts with your other PR

@zsarnett zsarnett self-assigned this May 23, 2022
@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented May 23, 2022

It looks much much better (eg. ui.components.logbook.for is gone) but it still looks (based on the screenshots) that texts are concatenated together... It would be still difficult for translator to translate it in a way that make sense.

Something like this can be traslated more easier:

Light turned on
Triggered by automatition: Some automatition name
Triggered by state of: Some entity

Unfortunately, while it is easier to translate, splitting makes it look inconstant which means it would need a UX redesign which is outside the scope of this PR.

Since this is an improvement over the last iteration, and it will make it easier to refactor into a multi-line setup later. I suggest we merge this now and iterate on the UX design in the future.

@zsarnett zsarnett merged commit a02b817 into home-assistant:dev May 23, 2022
@kukulich
Copy link
Copy Markdown
Contributor

Since this is an improvement over the last iteration, and it will make it easier to refactor into a multi-line setup later. I suggest we merge this now and iterate on the UX design in the future.

I totally agree 👍 It is already an improvement 👍 I just hope that the UX redesign will not be forgotten :)

@github-actions github-actions bot locked and limited conversation to collaborators May 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Broken localization of logbook messages (and others)

5 participants