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

Resolved Issue #1695: Sample values in Chart activity are not initialized #1696

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

kumar-piyush12
Copy link

Resolved: Sample values in Chart activity are not initialized #1695

Note: Only changes are done in onJournalNewInstance() and localized() of file in activities/Chart.activity/js/activity.js

1

I sincerely apologize for any indentation errors in activity.js. Despite my best efforts, I was unable to resolve them. As I am new to Open Source Contribution, I appreciate your understanding and am committed to improving in this area moving forward. Thank you for your consideration.

Summary of Issue: In Charts section of Desktop Sugarizer, incorrect labels such as 'EgLabel6' were showing, instead of labels of Sports names (e.g. Basketball, Tennis, Cricket, etc.)

Explanation of my approach:

  1. After reviewing the ChartView.js file, it is observed that the chart labels are derived directly from the tabular data without being processed through the translation system. This behavior is identified within the computed section of the code.
    labels() { return this.tabularData.map((data) => data.x || ""); },

  2. This implies the necessity to identify where tabularData is being set. Given that this is a Vue component that receives tabularData as a prop, it is crucial to examine either:

  • The parent component that is responsible for setting this data, or
  • activity.js, which is likely the script where the initial data is being prepared.
  1. After reviewing the activity.js, it can be modified to ensure the labels are properly translated using the localization system before being passed to the chart component. This would involve utilizing the translations from the en.json and fr.json files to convert labels such as "EgLabel6" to "Chess" and "EgLabel2" to "Cricket," among others.
onJournalNewInstance() {
    const randArr = this.shuffleArray([1, 2, 3, 4, 5, 6]);
    for(let i = 0; i < 3; i++) {
        const label = "EgLabel" + randArr[i];
        this.tabularData.push({
            x: this.SugarL10n.get(label),  // This should translate but isn't working
            y: randArr[i] + 6 + "",
        })
    }
    this.pref.labels.x = this.SugarL10n.get("Sports"); 
    this.pref.labels.y = this.SugarL10n.get("Students");
    // ...
}
  • Above is the problematic code in activity.js, the issue appears to be in the onJournalNewInstance method, where the labels are being created but the translation is being handled incorrectly. The problem may stem from two potential causes:
  • The localization system (SugarL10n) may not be fully initialized when this code executes.
  • The translation function may not be functioning as intended.
  1. New Version of onJournalNewInstance() to ensure labels are properly translated:
onJournalNewInstance() {
    const randArr = this.shuffleArray([1, 2, 3, 4, 5, 6]);
    for(let i = 0; i < 3; i++) {
        const label = "EgLabel" + randArr[i];
        const translatedLabel = this.SugarL10n.get(label);
        
        // Add fallback in case translation fails
        this.tabularData.push({
            x: translatedLabel || label,
            y: randArr[i] + 6 + "",
        });
        
        // Log for debugging
        console.log(`Label: ${label}, Translated: ${translatedLabel}`);
    }
    this.pref.labels.x = this.SugarL10n.get("Sports"); 
    this.pref.labels.y = this.SugarL10n.get("Students");
    this.updateActivityTitle(this.l10n.stringChartActivity);

    const header = [this.pref.labels.x, this.pref.labels.y];
    this.$refs.csvView.updateJsonData(this.tabularData, header, true);
}
  1. New Version of localized() method to ensure translations are loaded:
    localized() {
      this.SugarL10n.localize(this.l10n)
      document.getElementById('export-csv-button').title =
        this.l10n.stringexportAsCSV
      document.getElementById('export-img-button').title =
        this.l10n.stringSaveImage

      // Add this to ensure translations are ready
      if (this.tabularData.length === 0) {
        this.onJournalNewInstance()
      } else {
        // Refresh existing labels with translations
        this.tabularData = this.tabularData.map((data) => ({
          ...data,
          x: data.x.startsWith('EgLabel') ? this.SugarL10n.get(data.x) : data.x,
          y: data.y,
        }))
      }
    },
  1. These changes will:
  • Ensure translations are loaded prior to usage.
  • Implement fallback behavior in case translations fail.
  • Add logging for better translation debugging.
  • Update existing labels once translations are available.
  • Enhance the robustness of the translation system.

@llaske
Copy link
Owner

llaske commented Jan 2, 2025

I sincerely apologize for any indentation errors in activity.js. Despite my best efforts, I was unable to resolve them.

You should change the settings in your IDE to avoid auto re-indent.
Only changed line should be in the PR.

@kumar-piyush12
Copy link
Author

kumar-piyush12 commented Jan 3, 2025

I sincerely apologize for any indentation errors in activity.js. Despite my best efforts, I was unable to resolve them.

You should change the settings in your IDE to avoid auto re-indent. Only changed line should be in the PR.

Thank you for helping me out, please have a look at my updated code. I hope it completely resolves the issue.

  • The commit is titled as 'fix chart activity'
  • Please let me know if further modifications are required.

image

@SnehalSrivastava27
Copy link

@kumar-piyush12 I feel there are some indentation done automatically by IDE in your new commit as well

Copy link

@SnehalSrivastava27 SnehalSrivastava27 left a comment

Choose a reason for hiding this comment

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

The line 147 to 152 which you changed should be same as there is indentation change only

@kumar-piyush12
Copy link
Author

The line 147 to 152 which you changed should be same as there is indentation change only
Hi Snehal, thanks for the review. I again tried to work on indentation issue, and I was able to ensure that only 'changed lines' are in PR.
But in the changed lines, I am still encountering 'internal spacing issue'. I tried my best but I could not resolve it.

I manually turned 'Auto Indent' settings to 'None' in VS-code. And it still shows some spacing issues in the 'changed lines'.
I humbly request @llaske to accept my PR request, and do let me know if any other issues arise.
I have made sure that Sugarizer runs properly with these changes
Commit title: 'fixed indentation activity.js'
image

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.

3 participants