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

Remove all user strings with instances of 'active/inactive' #8421

Merged
merged 2 commits into from Sep 14, 2021

Conversation

radinamatic
Copy link
Member

@radinamatic radinamatic commented Sep 12, 2021

Summary

For the time being I just commented out the relative pieces of code so the strings do not get extracted. Ideally we should delete them, but this should be enough to avoid them ending up on Crowdin.

My commenting out may not be the prettiest, but it seems to do the job, and at least I did not see any errors in the terminal or the browser console.

References

Fixes #8420

Reviewer guidance

Test if pages:

  • Reports > Lessons
  • Reports > Quizzes
  • Plan > Lessons
  • Plan > Quizzes

work as expected.


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@radinamatic radinamatic added TODO: needs review Waiting for review P1 - important Priority: High impact on UX TAG: user strings Application text that gets translated i18n Issues with fonts and translations in localizations DEV: frontend labels Sep 12, 2021
@radinamatic radinamatic added this to the 0.15.0 milestone Sep 12, 2021
Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

For the purposes of getting this in before String Freeze - go for it. I don't see anything here that would block that.

However, a follow-up issue should be made where a dev goes through and cleans up - not just the comments, but also to verify that all unnecessary functionality is removed - including one or more completely useless components.

I think that leaving the comments like so will be helpful in navigating that work so no need to remove them yet.

lessonActiveLabel: 'Active',
lessonInactiveLabel: 'Inactive',
},
// computed: {
Copy link
Member

Choose a reason for hiding this comment

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

This LessonActive component seems to be used in AssignmentSummary (not sure where in the app that is exactly but maybe in Coach > Plan somewhere)

This component has one job which is to say "Active" or "Inactive" based on the given value.

So this component should be refactored out of our app altogether or we should keep the strings to avoid them missing in AssignmentSummary.

Copy link
Member

Choose a reason for hiding this comment

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

Confirming now that we can also just remove AssignmentSummary as it is an unused component.

activeQuizLabel: 'Active',
inactiveQuizLabel: 'Inactive',
},
// computed: {
Copy link
Member

Choose a reason for hiding this comment

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

Same here as in the LessonActive changes. This is used in ReportsGroupReportQuizHeader currently so we'll need to account for that with the refactor by either adding or borrowing a newer string in place of this Active|Inactive one.

Copy link
Member

Choose a reason for hiding this comment

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

screenshot-localhost_8000-2021 09 14-09_54_30

ReportsGroupReportQuizQuestionListPage.vue and ReportsGroupReportQuizLearnerListPage.vue

have these headers for "status" and "average score" - @jtamiace safe to just remove the Status?

Seems also that there may be a bug where the average score isn't showing that seems unrelated to this PR.

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Holding this up for now until we can confirm that the strings we would want to update these to open/closed and visible to learner for quizzes/lessons are in a place where we can implement these changes after string freeze.

@rtibbles rtibbles dismissed their stale review September 14, 2021 21:47

I misunderstood that these strings are not actually displaying anywhere.

@rtibbles rtibbles merged commit a664ba8 into learningequality:develop Sep 14, 2021
@radinamatic radinamatic deleted the in-active-no-more branch September 14, 2021 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEV: frontend i18n Issues with fonts and translations in localizations P1 - important Priority: High impact on UX TAG: user strings Application text that gets translated TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants