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

Update select labels #1768

Merged
merged 8 commits into from
Sep 17, 2021
Merged

Update select labels #1768

merged 8 commits into from
Sep 17, 2021

Conversation

dominikx96
Copy link
Collaborator

Pull Request Template

Issue

Addresses #1761

  • This change addresses the issue in full
  • This change addresses only certain aspects of the issue
  • This change is a dependency for another issue
  • This change has a dependency from another issue

Description

Updates select options to use translations.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Prototype/POC (not to merge)
  • This change is a refactor/address technical debt
  • This change requires a documentation update
  • This change requires a SQL Script

How Can This Be Tested/Reviewed?

Open the "Add unit" form and check the "Unit Type" field options. They should be translated.

  • Desktop View
  • Mobile View
  • Test A
  • Test B

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have reviewed the changes in a desktop view
  • I have reviewed the changes in a mobile view
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have assigned reviewers
  • I have updated the changelog to include a description of my changes

@dominikx96 dominikx96 self-assigned this Aug 27, 2021
@netlify
Copy link

netlify bot commented Aug 27, 2021 •
edited
Loading

❌ Deploy Preview for dev-storybook-bloom failed.

🔨 Explore the source changes: 2522dc8

🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-storybook-bloom/deploys/6144fcfc9baee00007452cfd

@netlify
Copy link

netlify bot commented Aug 27, 2021 •
edited
Loading

✔️ Deploy Preview for dev-bloom ready!

🔨 Explore the source changes: 2522dc8

🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-bloom/deploys/6144fcfcb3d62a0008c9e25a

😎 Browse the preview: https://deploy-preview-1768--dev-bloom.netlify.app

@netlify
Copy link

netlify bot commented Aug 27, 2021 •
edited
Loading

✔️ Deploy Preview for dev-partners-bloom ready!

🔨 Explore the source changes: 2522dc8

🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-partners-bloom/deploys/6144fcfc4dc35a0008a8b714

😎 Browse the preview: https://deploy-preview-1768--dev-partners-bloom.netlify.app

@netlify
Copy link

netlify bot commented Aug 27, 2021

✔️ Deploy Preview for clever-edison-cd22c1 ready!

🔨 Explore the source changes: c09d4f5

🔍 Inspect the deploy log: https://app.netlify.com/sites/clever-edison-cd22c1/deploys/6128c97ccfc45300074631be

😎 Browse the preview: https://deploy-preview-1768--clever-edison-cd22c1.netlify.app

@netlify
Copy link

netlify bot commented Aug 27, 2021

✔️ Deploy Preview for clever-edison-cd22c1 ready!

🔨 Explore the source changes: 63dea39

🔍 Inspect the deploy log: https://app.netlify.com/sites/clever-edison-cd22c1/deploys/6128ca3d5d284a00071cec5b

😎 Browse the preview: https://deploy-preview-1768--clever-edison-cd22c1.netlify.app

emilyjablonski
emilyjablonski reviewed Aug 27, 2021
View reviewed changes
ui-components/src/locales/general.json
@@ -1032,7 +1032,14 @@
"individualUnits": "Individual Units",
"delete": "Delete this Unit",
"deleteConf": "Do you really want to delete this unit?",
"eligibility": "Eligibility"
"eligibility": "Eligibility",
"typeOptions": {
Copy link
Collaborator

@emilyjablonski emilyjablonski Aug 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

These translations already exist, we don't want to have any duplicates

Copy link
Collaborator

Choose a reason for hiding this comment

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

These translations still exist elsewhere under preferredUnit I think we can delete these added translations and use the others yeah?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@dominikx96 , can you please make this update.

Copy link
Collaborator

@seanmalbert seanmalbert Sep 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think the options for preferredUnit are a little different and we may not want a one to one here, since those have a + option.

emilyjablonski
emilyjablonski requested changes Aug 27, 2021
View reviewed changes
Copy link
Collaborator

@emilyjablonski emilyjablonski left a comment

Choose a reason for hiding this comment

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

See comment about translations :)

@dominikx96
Copy link
Collaborator Author

@pbn4 is there any option and sense to update keys: oneBdrm, twoBdrm, threeBdrm, fourBdrm to values that come from application.preferredUnit:

oneBedroom, twoBedroom, threeBedroom, moreThanThreeBedroom or should I map these values on frontend side?

@dominikx96 dominikx96 mentioned this pull request Aug 30, 2021
Closed
@dominikx96 dominikx96 added wip This PR is not ready for review, do not review it's a “Work In Progress” and removed ready for review labels Aug 31, 2021
@dominikx96 dominikx96 added ready for review and removed wip This PR is not ready for review, do not review it's a “Work In Progress” labels Sep 13, 2021
@seanmalbert
Copy link
Collaborator

@emilyjablonski , can you take a look at this again.

@emilyjablonski
Copy link
Collaborator

I commented two days ago on the duplicate translations still

@seanmalbert seanmalbert merged commit 8c35649 into dev Sep 17, 2021
@kathyccheng kathyccheng mentioned this pull request Sep 23, 2021
Closed
seanmalbert added a commit to CityOfDetroit/bloom that referenced this pull request Jun 23, 2022
@dominikx96 @seanmalbert
Update select labels (bloom-housing#1768)
aa35d4b 
* Update select labels

* Update changelog

* Remove duplicated translation

* Fix json structure

* updates translations and fixes unrelated issue with unit form

* reverts some changes on tranlsation keys

Co-authored-by: Dominik Barcikowski <dominik@airnauts.com>
Co-authored-by: Sean Albert <smabert@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seanmalbert seanmalbert seanmalbert left review comments

@emilyjablonski emilyjablonski Awaiting requested review from emilyjablonski

Assignees

@dominikx96 dominikx96

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dominikx96 @seanmalbert @emilyjablonski