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

Fix for HTML attributes being ignored for individual datetime selects. #5861

Merged
merged 2 commits into from Feb 12, 2015

Conversation

ADmad
Copy link
Member

@ADmad ADmad commented Feb 8, 2015

Closes #5829

@ADmad ADmad added this to the 3.0.0 milestone Feb 8, 2015
@ADmad ADmad added the defect label Feb 8, 2015
@markstory
Copy link
Member

Tests are failing.

@ADmad
Copy link
Member Author

ADmad commented Feb 8, 2015

@markstory Fixed code to ensure all tests pass but I am not satisfied with the change.

While this fixes problem for methods generating individual time selects you still can't pass attributes when using time() or datetime(). For that I think the attributes extraction I added to _singleDatetime() should be moved to _datetimeOptions() instead and also change should be done in DateTimeWidget.

Currently any extra attributes passed to $data of DateTimeWidget::render() are used as attributes for dateWidget template, which doesn't do much good since the default dateWidget does not have any {{attrs}} placeholder. So instead the attributes should be passed to calls done to SelectBoxWidget::render() calls for generating select tags for each datetime part.

@markstory
Copy link
Member

Is there much utility in having the same attrs applied to each select in a time/datetime compound input?

* @var array
*/
protected $_datetimeOptions = [
'interval', 'round', 'monthNames', 'minYear', 'maxYear',
Copy link
Member

Choose a reason for hiding this comment

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

What if each public method passed the list of exclusions to _singleDatetime()?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I had started doing earlier but then switched to using a common list at it was more convenient (lazy). Plus I had thought a single list would be required as I had planned to use it in _datetimeOptions() method.

@ADmad
Copy link
Member Author

ADmad commented Feb 9, 2015

Is there much utility in having the same attrs applied to each select in a time/datetime compound input?

Hmm someone might want same class applied to each select but apart from that can't think of any other, so perhaps not a good idea.

@markstory
Copy link
Member

One could always use a class on the wrapping div/container in that case.

@ADmad
Copy link
Member Author

ADmad commented Feb 10, 2015

One could always use a class on the wrapping div/container in that case.

Yes and one could add the wrapping div with class to the template so using the html attributes passed to datetime() as attributes for dateWidget template isn't very useful.

Instead not being able to specifically set class to select elements can be restrictive as some css framework might dictate that.

markstory added a commit that referenced this pull request Feb 12, 2015
Fix for HTML attributes being ignored for individual datetime selects.
@markstory markstory merged commit 443b94d into 3.0 Feb 12, 2015
@markstory markstory deleted the issue-5829 branch February 12, 2015 04:02
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.

None yet

2 participants