New Time Picker

[ExtremeFiretop]

New Time Picker? Inspired by the work I'm poking at for FlexQoS

Food for thought. Not ready yet anyways as the validator doesn't work correctly if I enter an invalid time/cron in the settings file.

[ExtremeFiretop]

@Martinski4GitHub

Any opinions?

[Martinski4GitHub]

@Martinski4GitHub

Any opinions?

Very nice and fancy! I like it!!! 👍👍

[ExtremeFiretop]

@Martinski4GitHub
Any opinions?

Very nice and fancy! I like it!!! 👍👍

Then I'll continue to poke at it some more!
Thanks for the feedback.

I've been busy myself in my personal life, put an offer on my first place which was accepted. I'm both very excited and very impatient 😂

Inspection is tomorrow, looking forwards to NOT finding anything hahaha!
The place is very clean and relatively new (early 2000's build with renos in 2018) so I doubt we will find anything personally.

So all that to say, I haven't had much time to poke at any of this work recently with everything going on, but wanted to run this idea by you while it was still in progress.

I'll finish it up with the validator and open the PR when ready :)

[ExtremeFiretop]

@Martinski4GitHub

Good to test / review :)
I think I've removed any dangling code from the previous solution but can always take a second look/pair of eyes!

[ExtremeFiretop]

While testing this, I realized our production code does not modify the cron job when modifying the time.
Since this bug exists in production I can safely assume I didn't introduce it here... Will review more.

[ExtremeFiretop]

Fixed production issue in commit: 47f45b6

[Martinski4GitHub]

@Martinski4GitHub
Any opinions?

Very nice and fancy! I like it!!! 👍👍

Then I'll continue to poke at it some more! Thanks for the feedback.

I've been busy myself in my personal life, put an offer on my first place which was accepted. I'm both very excited and very impatient 😂

Inspection is tomorrow, looking forwards to NOT finding anything hahaha! The place is very clean and relatively new (early 2000's build with renos in 2018) so I doubt we will find anything personally.

Congratulations on buying your first home!!! It's a major milestone in one's life, so I fully understand the excitement of it, and how you cannot wait to move in very soon. I wish you good luck with all the next steps in finalizing the purchase, as you will need to "dot the I's and cross the T's" during the financing process, inspections, moving logistics, etc...

P.S.
Apologies for the radio silence all this week. I've been extremely busy with work as I had some major deliverables due Friday, the 29th (just before our Labor Day 3-day weekend, YEAH!!!).

[Martinski4GitHub]

While testing this, I realized our production code does not modify the cron job when modifying the time. Since this bug exists in production I can safely assume I didn't introduce it here... Will review more.

Great catch!!!

[Martinski4GitHub]

@Martinski4GitHub

Good to test / review :) I think I've removed any dangling code from the previous solution but can always take a second look/pair of eyes!

I'm really mentally tired at the moment, after what felt like a very long work week, so I need to go back to sleep and will take a look at your PR changes sometime tomorrow evening (I woke up to go to the bathroom and decided to reply to some messages I had bookmarked as "pending").

Have a good night, bud!!!

[ExtremeFiretop]

Congratulations on buying your first home!!! It's a major milestone in one's life, so I fully understand the excitement of it, and how you cannot wait to move in very soon. I wish you good luck with all the next steps in finalizing the purchase, as you will need to "dot the I's and cross the T's" during the financing process, inspections, moving logistics, etc...

Thank you!!!! Not going to lie it felt like it may never happen with the cost of housing these days. Luckily once I quit smoking the extra money started to grow in my savings quicker. I just had to get past the first month of "Angry Joel" 😭 hahaha

The inspection went really well, everything found was small stuff I could fix myself over a summer. Things like caulking missing in some places... A single electrical outlet not functional. Stuff like that. 👌

The inspection did reveal one major red flag which was moisture in the basement walls.
But the current home owner as agreed to repair the issue at his cost. Had a foundation company give a quote of about 9000$ to fix the foundation and add a new water tight polyurethane membrane barrier against the foundation of the house. And he ripped open all the drywall and insulation from the basement to let things breath and air out in the meantime. (Which will all be replaced once the new membrane is installed of course)

P.S. Apologies for the radio silence all this week. I've been extremely busy with work as I had some major deliverables due Friday, the 29th (just before our Labor Day 3-day weekend, YEAH!!!).

Oh buddy don't even worry I kinda forgot about this PR once I opened it due to all the ongoing stuff right now between work and my personal life..Lol! We also get a 3 day weekend this weekend for labour day! But try not to do too much labour on labour day 😜
Defeats the purpose! Remember to take some time to relax this weekend, no one has reported any bugs so we are still good.

[ExtremeFiretop]

While testing this, I realized our production code does not modify the cron job when modifying the time. Since this bug exists in production I can safely assume I didn't introduce it here... Will review more.

Great catch!!!

How did this get introduced? I don't know.

I am almost positive we had tested this part of the WebUI back some versions .. but it is what it is, the small change in the commit I linked is all we needed to fix the issue. No rush to validate this of course, as I mentioned no one seems to have found this bug yet.

[Martinski4GitHub]

Congratulations on buying your first home!!! It's a major milestone in one's life, so I fully understand the excitement of it, and how you cannot wait to move in very soon. I wish you good luck with all the next steps in finalizing the purchase, as you will need to "dot the I's and cross the T's" during the financing process, inspections, moving logistics, etc...

Thank you!!!! Not going to lie it felt like it may never happen with the cost of housing these days. Luckily once I quit smoking the extra money started to grow in my savings quicker. I just had to get past the first month of "Angry Joel" 😭 hahaha

The inspection went really well, everything found was small stuff I could fix myself over a summer. Things like caulking missing in some places... A single electrical outlet not functional. Stuff like that. 👌

The inspection did reveal one major red flag which was moisture in the basement walls. But the current home owner as agreed to repair the issue at his cost. Had a foundation company give a quote of about 9000$ to fix the foundation and add a new water tight polyurethane membrane barrier against the foundation of the house. And he ripped open all the drywall and insulation from the basement to let things breath and air out in the meantime. (Which will all be replaced once the new membrane is installed of course)

P.S. Apologies for the radio silence all this week. I've been extremely busy with work as I had some major deliverables due Friday, the 29th (just before our Labor Day 3-day weekend, YEAH!!!).

Oh buddy don't even worry I kinda forgot about this PR once I opened it due to all the ongoing stuff right now between work and my personal life..Lol! We also get a 3 day weekend this weekend for labour day! But try not to do too much labour on labour day 😜 Defeats the purpose! Remember to take some time to relax this weekend, no one has reported any bugs so we are still good.

Well, on Saturday morning, my wife surprised me with an overnight stay trip to Santa Catalina Island, off the coast of Southern California, departing from Long Beach harbor on a catamaran ferry.

Oh, man, it was a fun trip!! And I really needed to unplug and get away from it all - just to chill, relax, and get out of our daily routine for a short while. We came back home late last night!

Anyway, now back to our regular days... I'll take a look at and review the PR code changes right now.

[Martinski4GitHub]

While testing this, I realized our production code does not modify the cron job when modifying the time. Since this bug exists in production I can safely assume I didn't introduce it here... Will review more.

Great catch!!!

How did this get introduced? I don't know.

I am almost positive we had tested this part of the WebUI back some versions .. but it is what it is, the small change in the commit I linked is all we needed to fix the issue. No rush to validate this of course, as I mentioned no one seems to have found this bug yet.

Yeah, I had the same impression that we had tested that part when it was first implemented, but we obviously missed something!! It happens... 😱🤨😉

[ExtremeFiretop]

Well, on Saturday morning, my wife surprised me with an overnight stay trip to Santa Catalina Island, off the coast of Southern California, departing from Long Beach harbor on a catamaran ferry.

Oh, man, it was a fun trip!! And I really needed to unplug and get away from it all - just to chill, relax, and get out of our daily routine for a short while. We came back home late last night!

Lucky man, sounds ike your wife seems to know how to treat you when it's time to unplug! Sounds like a fantastic trip, and honestly who wouldn't want to spend a few days away on a long weekend!

I'm happy you got to unwind a bit it sounds like you had a big week and some time not thinking about any work or code probably helps refresh the mind 😜

Anyway, now back to our regular days... I'll take a look at and review the PR code changes right now.

Welcome back! No rush, I'm hoping you find a few things to cleanup like you always do 😉 but truthfully I think I got "most" of the dangling lose ends in the code.

[ExtremeFiretop]

While testing this, I realized our production code does not modify the cron job when modifying the time. Since this bug exists in production I can safely assume I didn't introduce it here... Will review more.

Great catch!!!

How did this get introduced? I don't know.
I am almost positive we had tested this part of the WebUI back some versions .. but it is what it is, the small change in the commit I linked is all we needed to fix the issue. No rush to validate this of course, as I mentioned no one seems to have found this bug yet.

Yeah, I had the same impression that we had tested that part when it was first implemented, but we obviously missed something!! It happens... 😱🤨😉

Thinking back, I think I tested to validate the settings file got updated, which it still does. But I don't remember if I ever actually checked that it was changing the cron jobs... Maybe that was our oversight at the time.

Like you said, it happens, we can't be expected to be perfect. It's why a team is better than any single person. We get the opportunity to catch and refine this stuff before the end user reports anything hahaha 🤣

[Martinski4GitHub]

Well, on Saturday morning, my wife surprised me with an overnight stay trip to Santa Catalina Island, off the coast of Southern California, departing from Long Beach harbor on a catamaran ferry.
Oh, man, it was a fun trip!! And I really needed to unplug and get away from it all - just to chill, relax, and get out of our daily routine for a short while. We came back home late last night!

Lucky man, sounds ike your wife seems to know how to treat you when it's time to unplug!

Yeah, she notices the little signs when I'm overstressed, coming back home from work mentally exhausted, and I just fall asleep on the couch before or just after dinner while watching a movie or TV show, or when I don't feel like eating a food that I really enjoy with much gusto!! So yes, I got lucky with my "partner in crime"!! 😉😆

Sounds like a fantastic trip, and honestly who wouldn't want to spend a few days away on a long weekend!

I'm happy you got to unwind a bit it sounds like you had a big week and some time not thinking about any work or code probably helps refresh the mind 😜

Yes, exactly!! It feels great to have completely unplugged, even if only for a couple of days, to "recharge the batteries and fill the tank" to have another go around at a busy life!!!

[Martinski4GitHub]

While testing this, I realized our production code does not modify the cron job when modifying the time. Since this bug exists in production I can safely assume I didn't introduce it here... Will review more.

Great catch!!!

How did this get introduced? I don't know.
I am almost positive we had tested this part of the WebUI back some versions .. but it is what it is, the small change in the commit I linked is all we needed to fix the issue. No rush to validate this of course, as I mentioned no one seems to have found this bug yet.

Yeah, I had the same impression that we had tested that part when it was first implemented, but we obviously missed something!! It happens... 😱🤨😉

Thinking back, I think I tested to validate the settings file got updated, which it still does. But I don't remember if I ever actually checked that it was changing the cron jobs... Maybe that was our oversight at the time.

Like you said, it happens, we can't be expected to be perfect. It's why a team is better than any single person. We get the opportunity to catch and refine this stuff before the end user reports anything hahaha 🤣

Indeed. LOL!!!

[Martinski4GitHub]

@ExtremeFiretop,

I installed the latest files on my test router (RT-AC86U), and on the webUI, I do get the new "time picker", but there are no drop-down menu options to select the time parameters, so it does not match your screenshot. Right now, the only input option is to type in the values, which is not necessarily bad, but I was expecting to see the drop-down menu. Any idea what's going wrong?

BTW, I'm using the latest Firefox browser (142.0.1) on my Win11 laptop. Running latest RMerlin F/W 3004.386.14.2 version on the router.

P.S. No errors on the Browser Console window either.

[Martinski4GitHub]

While testing this, I realized our production code does not modify the cron job when modifying the time. Since this bug exists in production I can safely assume I didn't introduce it here... Will review more.

Please take a look at PR #515. This fixes the root cause of the problem you found. Again great catch!!

[Martinski4GitHub]

Ready for testing again :)

OK, sounds good. Let me grab the latest file to retest and validate.

[Martinski4GitHub]

Ready for testing again :)

The new functionality is looking very good.; not as "fancy" as the original proposal, but it works well and it does the job.
I have only 2 nitpick suggestions:

1. Moved the "AM" string closer to the "minute" value.
2. Remove/decrease the unused blank space between the time box and the new drop-down hour menu selection.

[Martinski4GitHub]

@ExtremeFiretop,

The new webUI time schedule implementation looks logically & functionally correct. So far, my latest testing and validation have found no errors or issues. However, a lot of the JavaScript code fails to comply with rules and guidelines for best coding practices and recommended coding standards.

1. Using one-letter and two-letter variables is no longer considered good coding practice. The latest and most modern best coding practices and standards state that variables should have meaningful names that convey their purpose, and/or the kind of data they contain. The best practice standard rule is that variable names should have at minimum 3 chars (e.g. msg="", cnt=0, ind=0, used for simple loops), but longer are recommended to make the code easier to review, check, and analyze for correctness, cohesion, and appropriate scope. Ultimately, using meaningful variable names makes the code easier to refactor and maintain.

2. Very long code lines (over ~80-90 chars) are no longer considered valid practice. The idea is to split the code line into more manageable, shorter lines to increase readability and make the code easier to review, check, and analyze. Studies have been conducted that prove that a series of shorter code lines works exceedingly well at improving readability and focus (for review and maintenance), and at preventing cognitive overload when compared with a series of very long code lines using the same logic flow.

Now, to be fair, you were likely not aware of all these best coding practices and standards, but I'm letting you know so you can improve and fine-tune your coding skills. Also, it looks like you may not have originally created a lot of the code used in the implementation because the coding style looks very different from the usual code that you write from scratch. It's fine to reuse code, but it's always recommended to update the code to the latest best practices once the implementation is working well; otherwise, it will eventually lead to an increase in technical debt.

My 2 cents.

[ExtremeFiretop]

@ExtremeFiretop,

The new webUI time schedule implementation looks logically & functionally correct. So far, my latest testing and validation have found no errors or issues. However, a lot of the JavaScript code fails to comply with rules and guidelines for best coding practices and recommended coding standards.

1. Using one-letter and two-letter variables is no longer considered good coding practice. The latest and most modern best coding practices and standards state that variables should have meaningful names that convey their purpose, and/or the kind of data they contain. The best practice standard rule is that variable names should have at minimum 3 chars (e.g. msg="", cnt=0, ind=0, used for simple loops), but longer are recommended to make the code easier to review, check, and analyze for correctness, cohesion, and appropriate scope. Ultimately, using meaningful variable names makes the code easier to refactor and maintain.
2. Very long code lines (over ~80-90 chars) are no longer considered valid practice. The idea is to split the code line into more manageable, shorter lines to increase readability and make the code easier to review, check, and analyze. Studies have been conducted that prove that a series of shorter code lines works exceedingly well at improving readability and focus (for review and maintenance), and at preventing cognitive overload when compared with a series of very long code lines using the same logic flow.

Now, to be fair, you were likely not aware of all these best coding practices and standards, but I'm letting you know so you can improve and fine-tune your coding skills. Also, it looks like you may not have originally created a lot of the code used in the implementation because the coding style looks very different from the usual code that you write from scratch. It's fine to reuse code, but it's always recommended to update the code to the latest best practices once the implementation is working well; otherwise, it will eventually lead to an increase in technical debt.

My 2 cents.

I've addressed all this for each function, and hopefully you'll have an easier time reviewing the variables this round :)

[ExtremeFiretop]

Ready for testing again :)

The new functionality is looking very good.; not as "fancy" as the original proposal, but it works well and it does the job. I have only 2 nitpick suggestions:

1. Moved the "AM" string closer to the "minute" value.
2. Remove/decrease the unused blank space between the time box and the new drop-down hour menu selection.

Nit-picks! Nit nit nitty nit picks! ;)

I gotcha I'll submit it in a moment

[ExtremeFiretop]

Give that a try and let me know if it satisfies you more ;)

[Martinski4GitHub]

@ExtremeFiretop,
The new webUI time schedule implementation looks logically & functionally correct. So far, my latest testing and validation have found no errors or issues. However, a lot of the JavaScript code fails to comply with rules and guidelines for best coding practices and recommended coding standards.

1. Using one-letter and two-letter variables is no longer considered good coding practice. The latest and most modern best coding practices and standards state that variables should have meaningful names that convey their purpose, and/or the kind of data they contain. The best practice standard rule is that variable names should have at minimum 3 chars (e.g. msg="", cnt=0, ind=0, used for simple loops), but longer are recommended to make the code easier to review, check, and analyze for correctness, cohesion, and appropriate scope. Ultimately, using meaningful variable names makes the code easier to refactor and maintain.
2. Very long code lines (over ~80-90 chars) are no longer considered valid practice. The idea is to split the code line into more manageable, shorter lines to increase readability and make the code easier to review, check, and analyze. Studies have been conducted that prove that a series of shorter code lines works exceedingly well at improving readability and focus (for review and maintenance), and at preventing cognitive overload when compared with a series of very long code lines using the same logic flow.

Now, to be fair, you were likely not aware of all these best coding practices and standards, but I'm letting you know so you can improve and fine-tune your coding skills. Also, it looks like you may not have originally created a lot of the code used in the implementation because the coding style looks very different from the usual code that you write from scratch. It's fine to reuse code, but it's always recommended to update the code to the latest best practices once the implementation is working well; otherwise, it will eventually lead to an increase in technical debt.
My 2 cents.

I've addressed all this for each function, and hopefully you'll have an easier time reviewing the variables this round :)

Yes, the code is now much easier to read & review to follow the intended logic and purpose. Following best coding practices makes the code look more polished & professional as well, and, as a bonus, it passes through the Linter tool checks with fewer warnings and errors. Good job, bud!!

[Martinski4GitHub]

Give that a try and let me know if it satisfies you more ;)

So far, I've found no functional issues during testing and validation. However, the spacing within the "Time Picker" GUI elements is getting messed up in Chrome, for some reason. The other browsers I've tried look OK.

This is on Google Chrome:

This is on MS Edge:

This is on Firefox:

This is on Safari (iPadOS):

[ExtremeFiretop]

Give that a try and let me know if it satisfies you more ;)

So far, I've found no functional issues during testing and validation. However, the spacing within the "Time Picker" GUI elements is getting messed up in Chrome, for some reason. The other browsers I've tried look OK.

This is on Google Chrome:

This is on MS Edge:

This is on Firefox:

This is on Safari (iPadOS):

Darn it how does this happen! I told you I dislike these types of issues exactly for this. Trying to find a standard for everyone in a world of no standards.... I'm going to give it one more shot or we just close this PR.

Let me review this solution in more depth later today. The reason I'd like to fix it is because I was really leaning on this type of time picker for FlexQoS and if we can't get it right here, then I may have to rethink the FlexQoS implementation.

[ExtremeFiretop]

Cancelling this plan for now.
Not worth the trouble, but was fun to poke at :)

[ExtremeFiretop]

@Martinski4GitHub

And incase your curious my choosen path for FlexQoS is to just not worry about Firefox or any fallback method for it.

[Martinski4GitHub]

Cancelling this plan for now. Not worth the trouble, but was fun to poke at :)

Yeah, it was a fun UI widget to play with, but, apparently, it has not yet been adopted as a standard widget by all the major browsers (e.g. Firefox and Chrome).

[Martinski4GitHub]

@Martinski4GitHub

And incase your curious my choosen path for FlexQoS is to just not worry about Firefox or any fallback method for it.

Based on future feedback, you'll find out if many users have Firefox or Chrome as their go-to browser when logging into the router webUI.

[ExtremeFiretop]

Cancelling this plan for now. Not worth the trouble, but was fun to poke at :)

Yeah, it was a fun UI widget to play with, but, apparently, it has not yet been adopted as a standard widget by all the major browsers (e.g. Firefox and Chrome).

I liked the widget a lot (especially the mobile versions)
But as you said; sadly it's not adopted by all and I think that's really just an unfortunately thing I can't band-aid on our end without the possibility of making tons of technical debt.

[Martinski4GitHub]

Cancelling this plan for now. Not worth the trouble, but was fun to poke at :)

Yeah, it was a fun UI widget to play with, but, apparently, it has not yet been adopted as a standard widget by all the major browsers (e.g. Firefox and Chrome).

I liked the widget a lot (especially the mobile versions) But as you said; sadly it's not adopted by all and I think that's really just an unfortunately thing I can't band-aid on our end without the possibility of making tons of technical debt.

Yeah, it was unfortunate. If you're keeping the "Time Picker" GUI implementation for FlexQoS, I'd suggest being upfront about the known possible issues and perhaps even include in your Release Notes a caveat that the UI element may have a comestic issue (in Chrome) or behave differently (in Firefox) from the expected functionality. Some screenshots, like the ones I posted, would clarify what the known issues are. This way, users are fully aware from the start and don't go into "panic mode" if they happen to see those issues, and report that "it doesn't work in my browser."

Just my 2 cents.

[ExtremeFiretop]

Cancelling this plan for now. Not worth the trouble, but was fun to poke at :)

Yeah, it was a fun UI widget to play with, but, apparently, it has not yet been adopted as a standard widget by all the major browsers (e.g. Firefox and Chrome).

I liked the widget a lot (especially the mobile versions) But as you said; sadly it's not adopted by all and I think that's really just an unfortunately thing I can't band-aid on our end without the possibility of making tons of technical debt.

Yeah, it was unfortunate. If you're keeping the "Time Picker" GUI implementation for FlexQoS, I'd suggest being upfront about the known possible issues and perhaps even include in your Release Notes a caveat that the UI element may have a comestic issue (in Chrome) or behave differently (in Firefox) from the expected functionality. Some screenshots, like the ones I posted, would clarify what the known issues are. This way, users are fully aware from the start and don't go into "panic mode" if they happen to see those issues, and report that "it doesn't work in my browser."

Just my 2 cents.

Very good point, I do plan on moving forwards with this solution on FlexQoS but I will document the fact that on Firefox specifically the box won't have a Widget to input the time.

I tested Chrome and it works fine with the Widget on FlexQoS and appears fine as well, that seemed to be something only here in MerlinAU for some reason, and I didn't have the "umph" in me to keep going down the rabbit hole when we have something that works perfectly fine as is. I would only be pushing for this change in MerlinAU for what?
For the few people that use their phones to get a fancy time picker? Forget it.. Not worth breaking my back troubleshooting over and over

Sadly for FlexQoS I wouldn't have the space for our current solution found in MerlinAU; so my only real option is to ignore FireFox and hope they get in line some day.