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

Timezone select #2091

Merged
merged 7 commits into from Jul 12, 2019
Merged

Timezone select #2091

merged 7 commits into from Jul 12, 2019

Conversation

MGatner
Copy link
Member

@MGatner MGatner commented Jul 9, 2019

Description
This adds a handy function to the sparse date_helper to generate a select field from PHP's available timezones. Takes optional parameters for HTML class, default value, and timezone filters.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

Copy link
Member

@lonnieezell lonnieezell left a comment

Choose a reason for hiding this comment

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

I like the idea and think it would make a good contribution. However, I don't believe that just using DateTimeZone::listIdentifiers() alone is a user-friendly solution. No one thinks, "My timezone is America/Chicago" :)

Here's a function I put together a couple of years ago for a project that just needed North America timezones. It ends up with a sorted list with each entry like CST - America/Chicago. So, it allows people to quickly find the right section by what they're used to (CST), but is still specific enough to work with all of the stuff we need to in PHP. I don't know how well it works outside of North America, honestly, but I think it should be good. Feel free to use and modify as needed:

public static function northAmerica()
    {
        $zones = DateTimeZone::listIdentifiers(DateTimeZone::PER_COUNTRY, 'US');

        // Get "common" prefix for each
        $newZones = [];
        foreach ($zones as $zone)
        {
            $date = new \DateTime('now', new \DateTimeZone($zone));
            $short = $date->format('T');

            if (! isset($newZones[$short]))
            {
                $newZones[$short] = [];
            }
            $newZones[$short][] = $zone;
        }

        // Sort the keys
        ksort($newZones);

        // Create the final list which includes the true name as the key
        // and a display name that incorporates the short name in it...

        $finalZones = [];
        foreach ($newZones as $short => $list)
        {
            foreach ($list as $zone)
            {
                $finalZones[$zone] = "{$short} - {$zone}";
            }
        }

        return $finalZones;
    }

@MGatner
Copy link
Member Author

MGatner commented Jul 12, 2019

I posted the updated code implementing your format, but I'm not sure that it produces a more desirable global solution. "Timezone abbreviation" doesn't appear to be defined for all valid timezones so falls back to a time offset (+/- #) which itself is inconsistent between ## and #### depending on whole/partial hour and causing #### to get sorted last (probably could be fixed by a sort flag to ksort()).

At the end of the day I think any given user will probably only be selecting a timezone once so it isn't a huge UI hit either way, and given the "messiness" of the problem to begin with we might be picking the lesser evil.

@lonnieezell
Copy link
Member

Oh, well damn. yeah if doesn't provide a timezone abbreviation for everyplace globally that just creates an even nastier solution mixing offset and abbreviation so sounds like we should scrap that and go back to what you had. Sorry about that!

I guess that explains with installing an OS typically shows a little map of the world with a few cities highlighted. Do you want to make that happen :) Kidding.

@MGatner
Copy link
Member Author

MGatner commented Jul 12, 2019

I could probably have the map done before the Email Library is ready. :O (Sorry Jim)

I will revert to the original version. Thanks for the input though, yours is an elegant solution for zones that support the abbreviation.

@lonnieezell
Copy link
Member

Actually - the old email lib is back in the repo now so we actually have email again. Yay! So the ball's in my court now to finish up the Migration stuff - which I'll have to get to next week after I get back from a small vacation.

@MGatner
Copy link
Member Author

MGatner commented Jul 12, 2019

I saw that. I also saw that the rewrite is planned for 4.1, which makes me hesitant to implement it too heavily in modules lest the functions change. A discussion for a different place, for sure...

@lonnieezell
Copy link
Member

Understandable

@lonnieezell lonnieezell merged commit cc60dad into codeigniter4:develop Jul 12, 2019
@MGatner MGatner deleted the timezone-select branch July 12, 2019 15:16
@jim-parry
Copy link
Contributor

The current "ported Email" is a dog's breakfast, IMHO.
It is there to say we have it, but I wouldn't flaunt it :-/

@MGatner
Copy link
Member Author

MGatner commented Jul 12, 2019

:) Well it is better than nothing, or leaving it open to slews of third-party modules post-release (cough auth cough). I'm looking over the code and CI4 wrappers now and am feeling more hopeful that modules could use the email Service and need not too many changes after a proper CI4 library version of Email hits.

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.

None yet

3 participants