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

Selecting an AM will redirect the datepicker view to a PM for 24h timeMode. #62

Closed
YussufElarif opened this issue Nov 7, 2017 · 12 comments

Comments

@YussufElarif
Copy link

YussufElarif commented Nov 7, 2017

I am using create-react-app as my development template. It has json-loader and i've imported everything as per the guide. onTimeChange, it calls a function to set the time to the state, which then feeds back into the TimePicker to via the time prop to update the view.

    handleTimeChange(time) {
        console.log(time)
        this.setState({ time });
    }

    render() {
        const { time } = this.state;

        return (
            <div className="form-group">
                <TimePicker
                    theme="classic"
                    time={time}
                    timeMode="24"
                    onTimeChange={this.handleTimeChange.bind(this)} />
            </div>
        );
    }

The problem comes in when I select the time, such as 1:00. I've logged what i get back from the onTimeChange method to compare the results between the view and the console.
image

and here is me selecting 13:00
image

both examples change the date picker time to 13:00, i'm just confused as to why it does it on the first one?

@ecmadao
Copy link
Owner

ecmadao commented Nov 7, 2017

Seems a bug. Could you tell me which theme are you using? The Material Theme(default) or the Classic one? And you are using 24 hours mode, right? Thanks for your feedback!

@YussufElarif
Copy link
Author

YussufElarif commented Nov 7, 2017

I'm using classic theme and using this css
import 'react-times/css/classic/default.css';

P.S

image

    constructor() {
        super();
        this.state = {
            time: '15:15'
        };
    }

    handleTimeChange(time) {
        this.setState({ time });
    }

    render() {
        const { time } = this.state;

        return (
            <div className="form-group">
                {time}
                <TimePicker
                    time={time}
                    timeMode="24"
                    theme="classic"
                    onTimeChange={this.handleTimeChange.bind(this)} />
            </div>
        );
    }

Oh, and on initial load, it seems to be adding an extra hour from the time I've written in the state?

@ecmadao
Copy link
Owner

ecmadao commented Nov 8, 2017

It looks like a problem caused by moment-timezone, it return a wrong timezone and change your time by mistake when initial. By the way, it seems works well for me on Safari but broken on Chrome. I'll check it more carefully to see what really happened.

@ecmadao
Copy link
Owner

ecmadao commented Nov 8, 2017

I found these issues:

But after I update moment & moment-timezone to the newest version, moment timezone still works wrong. So I publish a new version V2.2.6 to fix this problem occasional: If user give TimePicker an exactly time props, such as "15:15", then I won't moment to guess user's timezone when initial.

Now new version V2.2.6 is already published, so you can update your dependency to check it out. THX!

@YussufElarif
Copy link
Author

image

Thank you, that seems to have fixed the issue for the insertion of the initial time. However, selecting 1:00 still renders 13:00.

image

@ecmadao
Copy link
Owner

ecmadao commented Nov 9, 2017

@vemuez
I see. Thank you! It's same problem as the initial one, and I forget to fix it. Now I have published new version V2.2.7 and fixed this problem.

@YussufElarif
Copy link
Author

YussufElarif commented Nov 9, 2017

Unfortunately, after installing 2.2.7, 1:00 still redirects to 13:00. I would like to contribute too, tried cloning the repository to reproduce the bug and find the problem, but i can't get the branch to work. It's after clicking 1:00am, where timeMode is set to 24, it displays 13:00 instead of 1:00 😢 .

This applies to all the AM times from 1:00am to 11:30am.

  • 1:00 -> 13:00
  • 2:00 -> 14:00
    ...
    ...
  • 11:00 -> 23:00

It converts 24hr AM times to 24hr PM times. I am using the classic theme, and 24 for timeMode.


Here's a quick demo

test

@ecmadao
Copy link
Owner

ecmadao commented Nov 11, 2017

So strange. It worked well for me so I can't reappearance this bug. But by looking your gif, it really looks like the bug which happened before v2.2.7. Can you reinstall this package, or clean browser cache? Cause it's only a little change from v2.2.6 to v2.2.7, and maybe node module system or browser cached the required file.
It's only a guess, and I'm still debugging to check if any other bug exist.

@YussufElarif
Copy link
Author

Yea, it is as you say. I had already removed the node_modules, upgraded from 2.2.6 to 2.2.7. But it turns out the browser had cached it. Don't know why i didn't think about that sooner. Thank you for the help 😄

@YussufElarif
Copy link
Author

Well, I've encountered, one more bug in this change actually. 12:00 and 12:30 changes to 00:00 and 00:30.

@YussufElarif YussufElarif reopened this Nov 11, 2017
@ecmadao
Copy link
Owner

ecmadao commented Nov 16, 2017

Sorry to reply you late. I had some personal trivia to deal some days ago, and now I'm coming back to fix this issue. First thank you so much for your enthusiasm to help me find these bugs. It's my fault that now find these problems, and now I make some changes for this module:

  1. If you send time props for react-times component, then when initialize, it won't use time zone to render current time.
  2. When user chose a time, component will not use time zone either (Cause users know what time they have chosen, so there is need to guess their time zone and recheck the time).
  3. So now, only by sending a timezone props can make component use time zone when render time.

Now I have made a new version 2.2.8 to publish these changes. Would you like to update your dependency and check again? Thanks!

@YussufElarif
Copy link
Author

No worries @ecmadao, Thank you for the time to look at this. It works now 😄.

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

No branches or pull requests

2 participants