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

v4 - Add methods for determining if elements are shown, visible, expanded #21085

Closed
wants to merge 1 commit into from
Closed

v4 - Add methods for determining if elements are shown, visible, expanded #21085

wants to merge 1 commit into from

Conversation

Johann-S
Copy link
Member

@Johann-S Johann-S commented Nov 4, 2016

I made this PR for this feature request in #17021 :

Add methods for determining if elements are shown. visible, expanded, etc #15573

  • Tooltips
  • Collapse
  • Modal
  • Documentation

They are a lots of work to do but if I can have some feedbacks from maybe @fat, @bardiharborow and @mdo It would be great 😄 because I'm not sure if I chose the best way to do this

@mdo
Copy link
Member

mdo commented Nov 28, 2016

Unsure how much feedback I can give you on this one :). I'm in favor of the added methods to help folks build a bit easier though. This also probably doesn't need to be done immediately for v4 and could be punted to v4.1 to help prioritize things.

@mdo mdo added the feature label Nov 28, 2016
@Johann-S
Copy link
Member Author

Yeah sure I agree plus it's a big change. My concerne is about the way I chose to do this
https://github.com/Johann-S/bootstrap/blob/c055840713d2a718f8c2f017287ad13f06a18974/js/src/tooltip.js#L619-L636

this.each(function () {
let data = $(this).data(DATA_KEY)
if (!data) {
let _config = typeof config === 'object' ? config : null

Choose a reason for hiding this comment

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

'_config' is never reassigned. Use 'const' instead prefer-const

this._isTransitioning = false

Choose a reason for hiding this comment

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

Trailing spaces not allowed no-trailing-spaces

@@ -342,7 +350,10 @@ const Tooltip = (($) => {

this.element.removeAttribute('aria-describedby')
$(this.element).trigger(this.constructor.Event.HIDDEN)

Choose a reason for hiding this comment

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

Trailing spaces not allowed no-trailing-spaces

@Johann-S
Copy link
Member Author

I have another solution we limit isShown method for just one target so we can get rid of the exception
like this :

      if (config === 'isShown' && this.length > 0) {
        if (this.length > 1) {
          throw new Error('isShown can be used only for one tooltip')
        }
        let data   = $(this).data(DATA_KEY)
        if (!data) {
          const _config = typeof config === 'object' ? config : null
          data = new Tooltip(this, _config)
          $(this).data(DATA_KEY, data)
        }
        return data[config]()
      }

@Johann-S
Copy link
Member Author

I close this PR, for now it's to early to work on that feature since we use jQuery

@Johann-S Johann-S closed this Jan 18, 2018
@Johann-S Johann-S deleted the elementsVisibility branch January 18, 2018 08:09
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.

3 participants