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

refactor: change type and methods in MediaRange.ts #6120

Merged
merged 8 commits into from
Dec 6, 2022
Merged

Conversation

hinzzx
Copy link
Contributor

@hinzzx hinzzx commented Nov 30, 2022

We did the following changes:

  • getCurrentRange now always returns a result, because it fallbacks to the default rangeSet, previously it was returning "null";
  • initRangeSet method now expects 2 params, instead of 3.
  • We completely changed the usage of the initRangeSet, instead of initRangeSet(name: string, borders: [600, 1100, 1400], names: [S, L, M]), we now use it like:
initRangeSet(name: string, range: Map<string, Array<number>>);

Related to: #6080

@hinzzx hinzzx requested review from nnaydenow and ilhan007 November 30, 2022 15:31
@ilhan007
Copy link
Member

HI @hinzzx, thank you for the change,
just document better the refactoring of getCurrentRange and initRangeSet methods, the rest is not that important - variable is renamed, code is moved.
What is important is how the logic changed, the rest can be removed from the description:

  • getCurrentRange now always returns a result, because fallbacks to the default range set, previously it used to return "null"
  • initRangeSet - here we change completely the usage, the function signature is different (3 params previously, now it expects 2), instead of initRangeSet(name: string, borders: [600, 1100, 1400], names: [S, L, M]), we now use it initRangeSet(name: string, map: Map(s: [0, 600], m: [600, 1100], ...))

These are the important things to document, you can use the above description as a base and build on it, paraphrase it, etc..

Thank you, ilhan

@ilhan007
Copy link
Member

ilhan007 commented Nov 30, 2022

In addition, we can notice (from the build failure details) that several Media Gallery tests are failing due to our refactoring, you will have to check and fix the root cause:

MediaGallery layout
[chrome 107.0.5304.110 linux #0-6] ✓ auto layout S size
[chrome 107.0.5304.110 linux #0-6] ✖ auto layout M size
[chrome 107.0.5304.110 linux #0-6] ✖ auto layout L size and restricted height
[chrome 107.0.5304.110 linux #0-6] ✖ auto layout L size and unrestricted height

@@ -83,7 +83,7 @@ const getCurrentRange = (name: string, width = window.innerWidth): string => {
let currentRangeName;

rangeSet.forEach((value, key) => {
if (width >= value[0] && width <= value[1]) {
if (Math.floor(width) >= value[0] && Math.floor(width) <= value[1]) {
Copy link
Member

Choose a reason for hiding this comment

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

Better introduce a new const for "Math.floor(width)" before the loop, because now we call "Math.floor" twice per iteration , while it can be calculated just once.
f.e.
const effectiveWidth = Math.floor(width)

Copy link
Contributor Author

@hinzzx hinzzx Dec 3, 2022

Choose a reason for hiding this comment

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

Hi Ilhan, thanks for the tips and recommendations,

After some (manual) testing and debugging, I came to the following conclusion:

Following the logic that width: 1023px => "S" size and width: 1023.5px => M size, I found out that even without our change with the Math.floor(), the width value, our method getCurrentRange recieves is always rounded ( with Math.round logic, which returns the bigger integer after .5 and the smaller one below .5 ). With that said I even think that we might need to revert the Math.floor() change in order to keep the old logic, since our method never recieves a floating number.

Please correct me, if Im wrong.

Best regards,
Stoyan

@hinzzx hinzzx merged commit c937e89 into main Dec 6, 2022
@hinzzx hinzzx deleted the research-media-range branch December 6, 2022 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants