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

US152268 - Migrate calendar vdiff tests #3986

Merged
merged 7 commits into from
Sep 12, 2023

Conversation

svanherk
Copy link
Contributor

@svanherk svanherk commented Sep 7, 2023

Putting this up as a draft PR now, but switching of the language is still broken and needs to be fixed.

Final report: https://vdiff.d2l.dev/BrightspaceUI/core/fb38fc32295b9e57bf0b688111eb22f380269d4e/2023-09-08_17-55-03/report/

@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2023

Thanks for the PR! 🎉

We've deployed an automatic preview for this PR - you can see your changes here:

URL https://live.d2l.dev/prs/BrightspaceUI/core/pr-3986/

Note
The build needs to finish before your changes are deployed.
Changes to the PR will automatically update the instance.

@dlockhart dlockhart force-pushed the US152268_Migrate_calendar_vdiff_tests branch from e8b6a65 to 1df2546 Compare September 8, 2023 17:51
@dlockhart dlockhart force-pushed the US152268_Migrate_calendar_vdiff_tests branch from 1df2546 to fb38fc3 Compare September 8, 2023 17:51
Copy link
Member

@dlockhart dlockhart left a comment

Choose a reason for hiding this comment

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

Should be all fixed up now -- added some explanations inline.

}
function getCalendarData() {
calendarData = {};
calendarData.descriptor = getDateTimeDescriptor();
Copy link
Member

Choose a reason for hiding this comment

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

The culprit was actually two separate places where we were saving local caches -- calendarData here and in getDateTimeDescriptorShared.

Previously the caches were invalidated in calendar's event handler for d2l-localize-resources-change, but because of the isolated fixtures that event fires before any calendars are even put on the page.

I'm going to separately look into adding a cache inside the @brighspace-ui/intl library, since it's in a much better place to invalidate it.

@@ -139,12 +139,6 @@ export function getDateNoConversion(value) {
return new Date(parsed.year, parsed.month - 1, parsed.date, parsed.hours, parsed.minutes, parsed.seconds);
}

let dateTimeDescriptor = null;
export function getDateTimeDescriptorShared(refresh) {
Copy link
Member

@dlockhart dlockhart Sep 8, 2023

Choose a reason for hiding this comment

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

I just noticed that Nova is using this -- here's the PR to remove it.

@dlockhart dlockhart marked this pull request as ready for review September 8, 2023 19:33
@dlockhart dlockhart requested a review from a team as a code owner September 8, 2023 19:33
Co-authored-by: github-actions <github-actions@github.com>
@svanherk svanherk enabled auto-merge (squash) September 12, 2023 19:33
@svanherk svanherk merged commit 9d32624 into main Sep 12, 2023
7 checks passed
@svanherk svanherk deleted the US152268_Migrate_calendar_vdiff_tests branch September 12, 2023 19:33
@ghost
Copy link

ghost commented Sep 12, 2023

🎉 This PR is included in version 2.146.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ghost ghost added the released label Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants