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.

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

#140 - add update and delete opinion features #144

Merged
merged 5 commits into from
Nov 23, 2023
Merged

Conversation

vojcc
Copy link
Contributor

@vojcc vojcc commented Sep 8, 2023

This should close #140.

@vojcc vojcc requested a review from a team as a code owner September 8, 2023 10:46
lang/pl.json Outdated Show resolved Hide resolved
@krzysztofrewak krzysztofrewak requested review from Letha0 and a team September 13, 2023 16:52
Copy link
Contributor

@EwelinaSkrzypacz EwelinaSkrzypacz left a comment

Choose a reason for hiding this comment

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

  1. Confirmation modal is not translated at all (only przycisk "Usuń")

image

  1. "Pole content nie może być dłuższe niż 250 znaków." - word "content" is not translated

  2. Maybe administrator should be able to delete all opinions? Or edit? We can make issue for that to discuss it and implement in the future.

@AleksandraKozubal
Copy link
Collaborator

@vojcc are you interested in finishing this pr?

@vojcc
Copy link
Contributor Author

vojcc commented Nov 8, 2023

@vojcc are you interested in finishing this pr?

@AleksandraKozubal, someone can take a wheel.

@AleksandraKozubal
Copy link
Collaborator

@vojcc are you interested in finishing this pr?

@AleksandraKozubal, someone can take a wheel.

@vojcc alright, @Lee0z is working on it.

@Lee0z
Copy link
Collaborator

Lee0z commented Nov 12, 2023

@EwelinaSkrzypacz

Confirmation modal is not translated at all (only przycisk "Usuń")

image

I've managed to translate the header of modal and cancel button.

Content of this modal came out a little bit tricky. It has to be universal as component, but for now it doesn't work, WIP.

Also found a bug when trying to delete city:
image

I translated the header, but that means every occurence of DeleteModal component needs to be revised. (Headers might be not translated). Should we make another issue?

"Pole content nie może być dłuższe niż 250 znaków." - word "content" is not translated

I can't find any proper solution to "Pole content nie może być dłuższe niż 250 znaków." for now.

Maybe administrator should be able to delete all opinions? Or edit? We can make issue for that to discuss it and implement in the future

Sure, but it needs to be discussed in new issue.

Copy link
Contributor

@EwelinaSkrzypacz EwelinaSkrzypacz left a comment

Choose a reason for hiding this comment

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

@Lee0z

  1. I don't have translated text in delete modal (my application language is Polish):

image

We can leave this problem for another issue, but please crate one

  1. You can translate word content in lang/pl/validation.php in array atrributes.

  2. Please make issue about managing opinions by administrator.

@Lee0z
Copy link
Collaborator

Lee0z commented Nov 13, 2023

Didn't commit changes yesterday.

Translations are still work in progress, noticed new bug - after opinion deletion, toast is not translated.
image

@EwelinaSkrzypacz
Copy link
Contributor

Didn't commit changes yesterday.

Translations are still work in progress, noticed new bug - after opinion deletion, toast is not translated. image

You can translate this in file lang/pl.json

@Lee0z
Copy link
Collaborator

Lee0z commented Nov 15, 2023

Toast is now translated.

Fixed translation on DeleteModal content.
image

Any ideas how to universally translate content to match polish grammar?
image

Copy link
Contributor

@EwelinaSkrzypacz EwelinaSkrzypacz left a comment

Choose a reason for hiding this comment

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

Translation are almost fixed. Please look at screenshot below

image

It should be here "usunięte"

image

"Delete country" is not translated. It should be here "usunięty"

@Lee0z
Copy link
Collaborator

Lee0z commented Nov 16, 2023

Maybe we should change DeleteModal's header to something like:

"Usunąć miasto (city name)?"
"Usunąć opinię?"
"Usunąć kraj (country name)?"

And in modal's content:
"Tej operacji nie można cofnąć"

This way, we'll avoid conflicting polish grammar.

@EwelinaSkrzypacz
Copy link
Contributor

For me it will be okay.

@Lee0z
Copy link
Collaborator

Lee0z commented Nov 18, 2023

Is it alright?:

  • when deleting opinion:
image
  • when deleting city:
image
  • when deleting country:
image

added trimming translationKey in renderHeader to ensure there is no space before question mark.

Copy link
Contributor

@EwelinaSkrzypacz EwelinaSkrzypacz left a comment

Choose a reason for hiding this comment

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

For me it's great 😄

@EwelinaSkrzypacz
Copy link
Contributor

@Lee0z you can merge this pr 😃

@Lee0z Lee0z merged commit f4113a6 into main Nov 23, 2023
3 checks passed
@Lee0z Lee0z deleted the #140-update-delete-opinion branch November 23, 2023 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add update and delete methods for opinions
6 participants