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

Needs Clone: CLDR-17560 improve Dashboard robustness #3775

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

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Jun 3, 2024

  • use second param to callback() to return err instead of global
  • popup err message if dashboard fails to load

CLDR-17560

  • This PR completes the ticket.

ALLOW_MANY_COMMITS=true

- use second param to callback() to return err instead of global
- popup err message if dashboard fails to load
@srl295 srl295 requested a review from btangmu June 3, 2024 17:16
@srl295 srl295 self-assigned this Jun 3, 2024
Copy link
Member

@btangmu btangmu left a comment

Choose a reason for hiding this comment

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

will {{ fetchErr }} automatically show fetchErr.value?

if so, then this all looks good

return;
}
this.locale = cldrStatus.getCurrentLocale();
this.level = cldrCoverage.effectiveName(this.locale);
if (!this.locale || !this.level) {
this.fetchErr = "Please choose a locale and a coverage level first.";
this.fetchErr.value =
"Please choose a locale and a coverage level first.";
Copy link
Member

Choose a reason for hiding this comment

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

Does this work right in conjunction with <p v-if="fetchErr" class="st-sad">{{ fetchErr }}</p>?

In other words, will {{ fetchErr }} automatically show fetchErr.value?

@btangmu btangmu self-requested a review June 6, 2024 14:58
Copy link
Member

@btangmu btangmu left a comment

Choose a reason for hiding this comment

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

Maybe make fetchErr.value explicit where fetchErr is referenced near top of DashboardWidget.vue? And/or add a comment. We should test carefully after merging since there have been changes to these files in the meantime for virtual scroller; still it looks to me like they shouldn't be in conflict.

The change from created() to mounted() looks potentially important, I wonder if it might affect the "blinky" appearance we were discussing...

@srl295
Copy link
Member Author

srl295 commented Jun 7, 2024

Maybe make fetchErr.value explicit where fetchErr is referenced near top of DashboardWidget.vue? And/or add a comment. We should test carefully after merging since there have been changes to these files in the meantime for virtual scroller; still it looks to me like they shouldn't be in conflict.

Right.

The change from created() to mounted() looks potentially important, I wonder if it might affect the "blinky" appearance we were discussing...

https://vuejs.org/guide/essentials/lifecycle.html actually, created() might be better so that it does NOT blink.

@srl295 srl295 changed the title CLDR-17560 improve Dashboard robustness Needs Clone: CLDR-17560 improve Dashboard robustness Oct 21, 2024
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.

2 participants