karrot-frontend
karrot-frontend copied to clipboard
Rename group description to group information
Closes #2356
What does this PR do?
Renames group description
field to information
. In #2356 it was explained that a user suggested this in the forum because the field is used for information, not a description.
I have a few questions:
- I updated the locale-en.json file. This documentation says not to add the key to the other files because they will be automatically added by the CI tool and translated. Does the CI tool also update the changed files?
- Should I also change the
publicDescription
field to information? - Are there changes to be made in the backend, and if so, could you point me in the right direction?
Currently there is one test failing. I am still working on solving that, but I wanted to update that I am actively working on this issue.
I updated the locale-en.json file. This documentation says not to add the key to the other files because they will be automatically added by the CI tool and translated. Does the CI tool also update the changed files?
Yes, you are right to just modify locale-en.json
, our tooling will take care of the rest.
Should I also change the publicDescription field to information?
No, I think that one still makes sense as a description.
Are there changes to be made in the backend, and if so, could you point me in the right direction?
We have two choices here:
- leave the field name as
description
and just change the frontend label - also modify the field in the database
Doing option 1 is simpler, and 2 could always be done in the future. For 2, would you be comfortable working on the backend, or would you want someone else to do that part?
Currently there is one test failing. I am still working on solving that, but I wanted to update that I am actively working on this issue.
The failing test is one of the snapshot tests (it renders bit of the frontend and checks the html is the same), when making changes to the UI this is expected to change (it means you can detect changes happening when you did not expect it). So in this case you need to update the snapshots, you can do that by running yarn test -u
(then add/commit the updated snapshot file).
Hope that helps!
@nicksellen Thank you for this feedback.
I'm not sure I'm able to modify the database at this time. Would you like me to make a new issue for this? I can look into it further if someone else does not claim it.
Hey @kkreine
I'm not sure I'm able to modify the database at this time. Would you like me to make a new issue for this? I can look into it further if someone else does not claim it.
I am happy to do this bit of work :) I'll submit a PR shortly for that.
I just tried out your branch, and there are a few more bits to complete:
- [ ] update
src/locales/locale-en.json
with changes- [ ] remove `GROUP.DESCRIPTION'
- [ ] remove `GROUP.DESCRIPTION_VERBOSE'
- [ ] add `GROUP.INFORMATION'
- [ ] add `GROUP.INFORMATION_VERBOSE'
- [ ] fix handling of information vs description field (more info in inline comment...)
- [ ] update branch from master (your branch has become outdated with changes from master, I suggest you merge master into your branch, if you're not sure how to do that, you can look it up, ask me here, ask somewhere else, or pop into the karrot chat)
- [ ] add an Information item as a menu item in the side nav (instead of the current obscure little icon)
I suggest you just do as many bits as you feel OK doing, and I'm happy to do any of the other bits :)
I did the backend part over here --> https://github.com/karrot-dev/karrot-backend/pull/1211
So when this is ready, I'll merge them at the same time.
Let me know if you need any help!
Hi @nicksellen I made these requested changes, thank you. Those two lines were meant to implement the change without the backend part complete, so I understand why they are not necessary now. I'm getting a failing snapshot with these changes, and I think this failure is because I'm running the tests without the backend changes. Any guidance would be appreciated.
I was able to resolve the merge conflicts, but I'm running into an issue with auth when running the application locally, so when I get that figured out, I can add information as a menu item
This pull request is marked as stale because it has not had any activity for 180 days.
If it's still important for you add a comment saying what it means to you, remove the stale label, and/or add the "important" label :)
However, if you just leave it like this, I'll close it in 30 days to help keep your pull requests relevant!
Thanks!