Skip to content

Number.parseFloat: function repeated access#37147

Closed
yyjiugui wants to merge 4 commits intotwbs:mainfrom
yyjiugui:master
Closed

Number.parseFloat: function repeated access#37147
yyjiugui wants to merge 4 commits intotwbs:mainfrom
yyjiugui:master

Conversation

@yyjiugui
Copy link

Number.parseFloat function repeated access

@yyjiugui yyjiugui requested a review from a team as a code owner September 14, 2022 10:30
Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

Good catch! Thanks for your PR :)

@julien-deramond
Copy link
Member

@GeoSot I let you double-check :)

@GeoSot
Copy link
Member

GeoSot commented Sep 14, 2022

On first view, I was ready to approve, but I got in seconds thoughts.
Short answer, better stay as already is :


Explanation:

According to https://developer.mozilla.org/en-US/docs/Web/CSS/transition-duration,
is valid to have the value transition-duration: 3s, 1s;

So I changed offcanvas css to :
image

Built the assets and added some hard-way debug helpers
image

Browsed to my local documentation offcanvases and opened one....
The results are not so encouraging
image

Unfortunately, it can bee seen that the results without the parseFloat, may be strings, and on my as it depends on me, is better not to mess js mathematical calculations, involving strings

However, @yyjiugui your effort is much appreciated, so please do not hesitate to suggest future improvements

@yyjiugui
Copy link
Author

Hello, I would like to ask, how can I fully test the code after I change the code to ensure that it will not affect other modules.

@XhmikosR
Copy link
Member

Yeah, this is intentional for the aforementioned reason.

@XhmikosR XhmikosR closed this Sep 15, 2022
@GeoSot
Copy link
Member

GeoSot commented Sep 15, 2022

Hello, I would like to ask, how can I fully test the code after I change the code to ensure that it will not affect other modules.

A nice question. 🤟

For this scenario, I suppose we would need to index.spec.js and add a new testCase.
The testCase should contain an element (to the fixture.innerHtml), having given style with a like transition-duration: 0.3s, 1s; .
After this, we would need to call getTransitionDurationFromElement using the HTMLelement we created, and to test that the result will be a float value (0.3)

We would be glad to accept a PR containing this test

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.

4 participants