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

docs(issues): Updated example for Drop-down component with associated input box #408

Closed
wants to merge 4 commits into from

Conversation

mschreib28
Copy link

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

  • Docs update

What is the current behavior? (You can also link to an open issue here)

  • Example missing second class example (when other dropdowns may exist)

What is the new behavior (if this is a feature change)?

  • Class added to div.dropdown so that compatibility with other dropdowns is maintained.

Does this PR introduce a breaking change?

  • No.

Please check if the PR fulfills these requirements

Other information:

@coveralls
Copy link

coveralls commented Feb 1, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 507bea8 on mschreib28:develop into 65350d3 on dalelotts:develop.

@dalelotts
Copy link
Owner

Thanks for the PR - can you elaborate a bit on Class added to div.dropdown so that compatibility with other dropdowns is maintained. - I honestly don't know when or why this additional class needs to be added and I think it would be helpful for the reader to understand why and when it is needed.

@mschreib28
Copy link
Author

From https://github.com/dalelotts/angular-bootstrap-datetimepicker#bootstrap-dropdown-using-jquery-3 :

To support multiple drop down's on a page, you need to make two changes:

This documentation change makes the ability to support multiple dropdowns supported by default, so new users of the directive don't get hung-up on the potential issue of if they use the dropdown class for things like menus in navigation or similar.

Please let me know if you need additional information. Thanks!

@dalelotts
Copy link
Owner

Ah, other dropdowns means multiple dropdowns on the same page. I see that on the readme now that I look at it.

I haven't played around with this PR, so I'll need your help. I'm trying to make sure I fully understand what problem this change is solving. Sometimes I'm a bit slow, I guess today is one of those days. =)

Okay, I see how changing the target to data-target=".date-dropdown" would prevent conflicts with the .dropdown class. I like it!

What issues do the other changes solve?

Finally, If a user pastes this new example into a page twice will the two dropdowns works independently?

@mschreib28
Copy link
Author

mschreib28 commented Feb 2, 2018

What issues do the other changes solve?

  • There were test failures on develop (someone changed the month to a 4-character month instead of the default 3-character month. The test changes fixes that.

Finally, If a user pastes this new example into a page twice will the two dropdowns works independently?

  • Unfortunately not. So we should probably mention that a unique identifier (class) is required for each drop-down

@mschreib28
Copy link
Author

@dalelotts - I've updated the README so that it uses IDs. While we can't force anyone to use unique ids for HTML elements, it is a best practice.

@dalelotts
Copy link
Owner

Thanks for all of the info!

I just realized that this PR is against the wrong repository for the angular 1.X code. This repo will be used for the Angular 5.x version.

The correct project is angularjs-bootstrap-datetimepicker

If I merge this PR here it will be gone when I merge the Angular 5.x changes.

Can you please create a PR for the angularjs-bootstrap-datetimepicker repository?

I am begging for your forgiveness for my mistake!

@dalelotts dalelotts closed this Feb 8, 2018
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