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

Update selector example codepen #32156

Closed
wants to merge 1 commit into from

Conversation

alpadev
Copy link
Contributor

@alpadev alpadev commented Nov 15, 2020

In reference to #28342

As requested by @Johann-S - Updated the codepen example in the popovers.md and tooltips.md

Updates the selector example codepen for popovers.md and tooltips.md, to reflect the lack of the jQuery dependency.
@XhmikosR
Copy link
Member

@mdo: we should fork that and use our Pen.

@alpadev
Copy link
Contributor Author

alpadev commented Nov 15, 2020 via email

@alpadev
Copy link
Contributor Author

alpadev commented Nov 15, 2020

Maybe I should mention that this example isn't supported in old IE because I used NodeList.forEach directly, instead of Array.forEach. I didn't know if you got any preferred way to do this e.g. [].forEach.call() vs Array.prototype.forEach.call().

Also I kept var button = null that could be simplified to var button.

@XhmikosR
Copy link
Member

RE: IE, that's OK, it's not supported on v5.

Feel free to simplify the pen and I'll fork it and update this PR after you are done. :)

@alpadev
Copy link
Contributor Author

alpadev commented Nov 15, 2020

@XhmikosR done. :)

@XhmikosR
Copy link
Member

@alpadev can you check https://codepen.io/team/bootstrap/pen/e1ebd72cfda6962c984dfd274e39658f please? I made a few more needed changes.

@alpadev
Copy link
Contributor Author

alpadev commented Nov 16, 2020

@XhmikosR looks much more polished now! Sorry for not putting more effort into it.

I agree it's better with some comments on those helpers.

About the code. The last minor updates I had done was to rename el to element on the toggle function, though I personally prefer el too. (I had some linter to complain about it recently.)

And for optimization I guess that

// Disable selector checkbox, put a tooltip on it, and show the buttons panel
    $("#use-selector").disabled = true;
    new bootstrap.Tooltip($("#use-selector-label span"));
    toggle($(".buttons"), true);

could be within the if-block because it needs to run only once imo. The jQuery example before had it outside the if-block (that way it's disabling the checkbox, initializing the tooltip and showing the buttons on each button press), maybe there is a reason to have it that way.

Maybe to make use of es6, one could replace the event handling functions with arrow-functions and the var with let, but thats like nitpicking. :)

Anyways, tested it and lgtm. 👍

@XhmikosR
Copy link
Member

I actually got rid of the toggle function and used our utility classes, please check one more time.

https://codepen.io/team/bootstrap/pen/e1ebd72cfda6962c984dfd274e39658f?editors=1010

The CodePen doesn't need to be perfect, it should just be a good demo :)

@alpadev
Copy link
Contributor Author

alpadev commented Nov 16, 2020

Makes sense. You could use it like that too,

// Demo-related
function updateCodeView() {
  $('#with-selector-code').classList.toggle('d-none', !usingSelectorOption());
  $('#without-selector-code').classList.toggle('d-none', usingSelectorOption());
}

but that's up to you. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants