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

Fix/1632/reset street layout global config #1633

Merged
merged 47 commits into from
Jan 11, 2021

Conversation

Med-H
Copy link
Contributor

@Med-H Med-H commented Dec 30, 2020

Reset layout option and street map max files in global settings

closes #1632
closes #1624

Description

GIVEN a user has opened the global config
WHEN modifying the layout or the street layout maximum treemap files, then resetting the global configuration
THEN layout and max treemap files options should revert to default values.

thuis PR closes this issue.

Med-H and others added 30 commits December 10, 2020 10:16
Copy link
Contributor Author

@Med-H Med-H left a comment

Choose a reason for hiding this comment

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

just modified the comment

@BridgeAR
Copy link
Member

BridgeAR commented Jan 7, 2021

This does not yet fix our flaky e2e tests:

FAIL app/codeCharta/e2e/url.e2e.ts (44.26 s)
  ● [secure] › should throw errors when file parameters in url are invalid and load sample data instead
    TimeoutError: waiting for selector `.md-select-menu-container.md-active > md-select-menu` failed: timeout 30000ms exceeded
      12 | 		await this.clickChooser()
      13 | 
    > 14 | 		await page.waitForSelector(".md-select-menu-container.md-active > md-select-menu")
         | 		           ^
      15 | 		const content = await page.$eval(".md-select-menu-container.md-active > md-select-menu", element => element["innerText"])
      16 | 		return content.split("\n")
      17 | 	}

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

I am a bit surprised that we reformat our changelog all the time. How could we guarantee this does not happen anymore? :-)

CHANGELOG.md Outdated

- Global settings not reverting to default ones ([#1632](https://github.com/MaibornWolff/codecharta/issues/1632))
- Maximum treemap files shown in squarified node ([#1624](https://github.com/MaibornWolff/codecharta/issues/1624))
- Fixed kotlin test ([#1322](https://github.com/MaibornWolff/codecharta/issues/1322))
Copy link
Member

Choose a reason for hiding this comment

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

This part should not be relevant for our customers. Therefore it's something we could in theory mention in "Chore" but I prefer not to add it at all.

Suggested change
- Fixed kotlin test ([#1322](https://github.com/MaibornWolff/codecharta/issues/1322))

private fun isFirstOrSecondWeek(kalenderWeeknWeek: Int): Boolean {
return kalenderWeeknWeek <= 2
private fun isFirstOrSecondWeek(calendarWeek: Int): Boolean {
return calendarWeek <= 2
Copy link
Member

Choose a reason for hiding this comment

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

❤️

.with(WeekFields.ISO.weekOfWeekBasedYear(), week.toLong())
.with(WeekFields.ISO.dayOfWeek(), 1)
.withHour(12).withMinute(0).withSecond(0).withNano(0)
.withOffsetSameInstant(ZoneOffset.UTC)
Copy link
Member

Choose a reason for hiding this comment

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

This is not immediately obvious what this does. I suggest to add a short comment what this tries to solve :-)

@@ -117,6 +117,7 @@ export class CodeMapLabelService implements CameraChangeSubscriber {
}
}

// TODO Add a test to ensure that scaling is not additive
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be unrelated?

@Med-H Med-H requested a review from BridgeAR January 8, 2021 11:25
}

private fun getWeekDate(year: Int, week: Int): OffsetDateTime? {
// returns the date of Monday based on the week and year
Copy link
Contributor

Choose a reason for hiding this comment

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

[Optional] I like to have longer variable names like getDateOfWeeksMonday instead of comments, which tend to rotten 😇 Guess that would also meet @BridgeAR wish for a comment.

// expect-puppeteer toClick timeout does not work it might be the reason of flaky tests,
// changed default options globally
// [toClick issue](https://github.com/smooth-code/jest-puppeteer/issues/202)
setDefaultOptions({ timeout: 6000 })
Copy link
Contributor

Choose a reason for hiding this comment

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

I have always a bad feeling with huge default timeouts 😟 But if it is necessary this shouldn't be a blocker

@shaman-apprentice shaman-apprentice merged commit 14df6a2 into main Jan 11, 2021
@shaman-apprentice shaman-apprentice deleted the fix/1632/reset-street-layout-global-config branch January 11, 2021 08:10
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.

reset street view global config not working The "maximum treemap files" is always visible
4 participants