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

Ericg/57 simplify playlist creation and updating #60

Merged

Conversation

@eric-gilbertson
Copy link
Collaborator

commented Sep 7, 2019

57: simplify edit & new playlist format

  • single page for playlist edit, delete & restorePlaylist
  • separate GET & POST operations for code clarity
  • normalize form element layout & styling
  • use single quote when possible
  • remove new DJ air name
  • round new show start time to now mod 1/4 hour
  • replace tab in playlist picker with a select element for active & deleted
  • remove tables check code

TODO

  • initialize new show start/end times to that of last week's show (if any)
  • decide whether new DJ Airname should be available in playlist edit screen
    or for clarity done in a different view?

Test Cases:

  • full CRUD on Playlists
  • edit play list elements, eg start time, show name ....
  • confirm add/remove track on a new playlists
  • any legacy operations missing in the new version?
eric-gilbertson and others added 14 commits Jun 27, 2019
merge with upstream
   - style editor widgets
   - insertTrack now returns 2 if the insertion was time stamped
   - don't call emitResponseHeader & emitBody if request is type 'appliction/json'
   * increase track editor input height
   * show user message to press tab or enter after inputing a tag id
   * invoke diskInfo query on blur out of tag id input
   * display user error if tag ID is invalid
   * show album/artist with a tag ID is entered as display only field
   * remove duration field in A file activity (was improperly displayed an no one noticed)
   * add dummy setFocus() function in order to make PHP happy
   * replace tag ID timeout logic and do ajax query on blur instead
   * use relative URLs for ajax queries
   * add spaces between album and label names to enable line wrapping
   - fix case where track title had a single quote char
   - upgrade to jquery 3.4.1 (same as is running on production) and remove version from file name
   - fix broken Print View link
   - include track artist when adding from a collection
   - store track titles in a global, not in the option value
   - clear track picker before track fetch and after adding a track
   - fetch disk tracks on track picker gaining focus (if it is empty)
   - remove duplicate jquery (again)
# Conflicts:
#	ui/Main.php
#	ui/Playlists.php
   * single page for playlist edit, delete & restorePlaylist
   * separate GET & POST operations for code clarity
   * normalize form element layout & styling
   * use single quote when possible
   * remove new DJ air name
   * round new show start time to now mod 1/4 hour
   * replace tab in playlist picker with a select element for active & deleted
   * remove tables check code

TODO
   * initialize new show start/end times to that of last week's show (if any)
   * decide whether new DJ Airname should be available in playlist edit screen
     or for clarity done in a different view?

Test Cases:
   * full CRUD on Playlists
   * edit play list elements, eg start time, show name ....
   * confirm add/remove track on a new playlists
   * any legacy operations missing in the new version?
    * add optional limit argument to IPlaylists::getPlaylists()
    * check for a show on same day last week & if so populate new PL values with it
    * set new PL start time to 1/4 hour if no show from last week to copy from (in Javascript)
@RocketMan

This comment has been minimized.

Copy link
Owner

commented Sep 8, 2019

Thanks Eric!

I have given it a quick once-over and observe the following:

  1. There is a geometry issue under Firefox. Webkit looks OK.

    Firefox:
    Screenshot from 2019-09-08 09-39-35

    Webkit:
    Screenshot from 2019-09-08 09-39-50

  2. Under Webkit, clicking in the Show Date field does not bring up the date picker. No messages are written to the Javascript console. Under Firefox, it is OK. I include a screenshot, just to confirm this is what is expected:
    Screenshot from 2019-09-08 09-42-23

  3. If there is supposed to be a time picker for Start Time and End Time, it is not appearing for me in either Firefox or Webkit.

  4. Can End Time be replaced with a duration control?

  5. The showtime value persisted in the database is not being normalized. Expected: The persisted showtime value should be normalized to ZZZZ-ZZZZ.

  6. The '(unpublished playlist)' option is missing from the DJ Air Name dropdown. Expected: '(unpublished playlist)' as the last option.

  7. Can we add '(create new airname)' or something along those lines to DJ Air Name, to replace the deprecated 'Setup New Airname...' button? It could appear at the bottom or top of the list.

Thanks Eric! If you cannot get to this before Monday, I can release the code from the last PR only (track editor changes) and we can follow-up with these changes later, or we can wait and release together, your call.

[Edit: Numbered the items for reference]

@eric-gilbertson eric-gilbertson changed the title Ericg/56 simplify playlist creation and updating Ericg/57 simplify playlist creation and updating Sep 8, 2019
eric.gilbertson added 2 commits Sep 8, 2019
   * restore add air name functionality & simplify logic
   * remove milliseconds from start/end timestampToAMPM
   * set default air name on playlist editPlaylist
   * add double click support for edit playlist picker

TODO:
fix date & time picker for european versions of firefox. recommend
deferral for now since it displays fine for US versions and this requires
introduction of a date picker component since the HTML5 version does not
work browsers.
@eric-gilbertson

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 8, 2019

Pushed updates per comments. All items resolved except for date picker not displaying properly in european versions of firefox.

@RocketMan

This comment has been minimized.

Copy link
Owner

commented Sep 9, 2019

Hello Eric,

Thanks for the quick turn-around. With the latest changes, I am see the following (points numbered in reference the previous comment):

  1. layout/clipping is still present. Very similar to this issue we encountered in the track edit screen. I see one css change in the latest two commits, but I do not believe it is related.

  2. Still not getting any date picker in webkit. Is there anything I can do to help debug this? There are no messages written to the console. Date picker works fine in Firefox.

  3. Webkit: As with (2) above, this one does not receive any special treatment in webkit, it is just a normal, free-form input field. I can type any random string in there, and it is accepted and stored to the db (see also number 5, below).

  4. Still open. If we can't do that, it's fine, but it might be more intuitive if the user can specify the show duration rather than end time.

  5. This one is still happening. It is the most serious of the issues, as it affects db integrity.
    We never scrubbed/normalized before, because the showtime was composed of fixed SELECT OPTION values. Expected: persisted 'showtime' value in the database should be NNNN-NNNN (e.g., 1330-1630)
    You can display or accept the time in any format, but it needs to be persisted in this way.

  6. For Edit Playlist, in the case of no airname (NULL), the first airname is being selected in the combo. Expected: 'None' should be selected if airname is NULL.

  7. Link is now present, which is fine. A couple issues:
    a. HREF is '/?session' but should be '?session' (no slash) to keep it relative.
    b. & in the QS needs to be &
    c. regression: when returning from the Add Airname page, the newly created airname should be pre-selected in the dropdown

Thanks Eric! I appreciate your diligence in making the UI improvements.

@RocketMan

This comment has been minimized.

Copy link
Owner

commented Sep 9, 2019

About (2) and (3): As it turns out, webkit does not support input type="date" or input type="time". That means Safari doesn't support them either, as confirmed here: https://html5test.com/compare/browser/safari-11.2.html

Can you replace the date and time pickers with something from jQuery or another source, since we cannot rely on browser support at this point?

   * fix timerange issue when storing to DB (#5)
   * set DJ air name to None if value is empty (#6)
   * use relative path in add airname link href (#7a)
   * fortify the time encode/decode logic
   * normalize layout of add air name format
@eric-gilbertson

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 10, 2019

pushed updates for the most significant issues. suggest retesting while i think about the remaining issues. regarding the clipped lines, it would be helpful if you could investigate the style differences between US and German version of the browser. Looks like too much padding in the latter.

@RocketMan

This comment has been minimized.

Copy link
Owner

commented Sep 11, 2019

Thanks Eric. I did some cursory testing on (5); it now looks good, though I'll test again once the pickers are sorted out.

As for (1), it affects only the browser-supplied date and time pickers; all the other fields are fine. Given that we are going to have to use Javascript pickers anyway, we can forget about this one.

   - inject DP if not support by browser, eg safari
   - show error message if edit post has error
   - reformat date to ISO on client when using DP
   - use hidden element to post real show date
@eric-gilbertson

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 13, 2019

Pushed addition of datepicker for safari. If it turns out that there are any European specific issues with it perhaps you can handle them since I have no way of reproducing them.

@RocketMan

This comment has been minimized.

Copy link
Owner

commented Sep 13, 2019

Thanks Eric.

As per the comment above, webkit/Safari doesn't have a time picker, either. If you have added one, I am not seeing it.

Issue (1) is not specific to European Firefox; it is caused by gtk3 theme styling, which could affect anyone on Linux/GNOME, depending on the theme. It can be fixed by massaging our css, if we stick with native pickers for Firefox.

Is there any reason why we would not want to use JavaScript pickers across all browsers? The native date picker in Firefox looks great, but the time picker is very, very minimal. Maybe you're looking at the time picker in Edge or Chrome, but in Firefox it is very basic: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/time Using a JavaScript control that looks like what Edge does natively (with sliding reels) would be ideal.

   - use jquery timepicker on safari
   - properly set default dj name on new show
@eric-gilbertson

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 14, 2019

hi jim,
per your request i have added time picker for safari. i didn't do it originally because i'm not sure it is any better than the native version, poor as it is. that is also why i think it is best to use native pickers where feasible. i'd note that the native date picker has been in use on the station web site for a year now and no one has reported any issues about it. furthermore, i'd note that the whole point of this change is to eliminate the need for DJs to enter anything when logging into a regular show so the pickers won't be used much.

this enhancement has been a long time in the making and i think it is time to put it up on staging for testing. can you do this prior to my show tomorrow (4:00 pm /Saturday) so that i can test it?

regards,
eric

@RocketMan

This comment has been minimized.

Copy link
Owner

commented Sep 14, 2019

Thank you, Eric! Date and time pickers now functional in webkit/safari!

i have added time picker for safari. i didn't do it originally because i'm not sure it is any better than the native version, poor as it is.

Just to be clear, there was no time picker in safari before your most recent commit; it does not support native pickers. There is no native time picker in safari, it is firefox which has the poor native picker.

Firefox pickers still have wonky geometry in the latest commit, but they can be massaged.

this enhancement has been a long time in the making and i think it is time to put it up on staging for testing. can you do this prior to my show tomorrow (4:00 pm /Saturday) so that i can test it?

The changes you made for the playlist editor have been deployed to stage since August 17. This PR is only one week old, and the last set of changes just arrived. However, I am going to go ahead and accept it and deploy to stage without the usual testing.

@RocketMan RocketMan merged commit 83ace23 into RocketMan:master Sep 14, 2019
RocketMan added a commit that referenced this pull request Sep 15, 2019
fixed #62 Add Air Name failures
fixed #61 Add Air Name is not preselecting added airname
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.