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

BSN 4.1 Feedback Required #430

Closed
thednp opened this issue Jan 17, 2022 · 42 comments
Closed

BSN 4.1 Feedback Required #430

thednp opened this issue Jan 17, 2022 · 42 comments
Labels
community question all in for feedback help wanted need your suggestion
Milestone

Comments

@thednp
Copy link
Owner

thednp commented Jan 17, 2022

I recently updated the BSN code base (click the link for a complete changelog) everybody is invited to test and provide feedback. I'm mostly interested to know any issue with Safari/Chrome on iOS, as I've tested everything on WIndows already.

@fmasa @midzer @cvaize @webstaurantstore @dtognazzini @PaulBGD and anyone else

Thank you and have a good one!

@thednp thednp added community question all in for feedback help wanted need your suggestion labels Jan 17, 2022
@thednp thednp added this to the 4.1 milestone Jan 17, 2022
@lekoala
Copy link
Contributor

lekoala commented Jan 21, 2022

dropdown positioning has improved dramatically! i had many issues with dropdowns in a sidebar, with this new version, it's more consistent to what i get with regular bootstrap 5 and popper.

@jcorporation
Copy link
Contributor

I upgraded to current master to help testing and I get a javascript error on loading:
Uncaught DOMException: Document.querySelector: '#' is not a valid selector

Error is in the last line of this function:

  function querySelector(selector, parent) {
    const selectorIsString = typeof selector === 'string';
    const lookUp = parent && parentNodes.some((x) => parent instanceof x)
      ? parent : getDocument();

    if (!selectorIsString && elementNodes.some((x) => selector instanceof x)) {
      return selector;
    }
    // @ts-ignore -- `ShadowRoot` is also a node
    return selectorIsString ? lookUp.querySelector(selector) : null;
  }

Any hints how I can debug this? Site works with BSN 4.0 flawlessly.

@thednp
Copy link
Owner Author

thednp commented Feb 11, 2022

Thanks for the feedback. I will test myself, perhaps selectors like #myElement don't work with the querySelector utility. You can post a screenshot of the expanded view of the error in the console.

@thednp
Copy link
Owner Author

thednp commented Feb 11, 2022

@jcorporation I've tested ID selectors on my setup and everything is fine, waiting for your confirmation and perhaps some sample code I can test with.

@jcorporation
Copy link
Contributor

Here is the screnshot:
image

The file that produces this error: https:/jcorporation/myMPD/blob/bsn41/htdocs/index.html

@jcorporation
Copy link
Contributor

jcorporation commented Feb 11, 2022

Found the error:
<a data-phrase="Move an output to this partition" class="dropdown-item featPartitions" data-bs-toggle="modal" href="#" data-bs-target="#modalPartitionOutputs"></a>

Must be:
<a data-phrase="Move an output to this partition" class="dropdown-item featPartitions" data-bs-toggle="modal" href="#modalPartitionOutputs"></a>

@thednp
Copy link
Owner Author

thednp commented Feb 11, 2022

Yes, that is a breaking change mentioned with each commit on the v4.1 branch. So we good now?

@thednp
Copy link
Owner Author

thednp commented Feb 11, 2022

May I suggest using this markup:

<a data-bs-toggle="modal" href="#modalPartitionOutputs"  class="dropdown-item featPartitions" data-phrase="Move an output to this partition"></a>

and

<button data-bs-toggle="modal" data-bs-target="#modalPartitionOutputs" class="dropdown-item featPartitions" data-phrase="Move an output to this partition" ></button>

Makes a lot more sense to keep consistency with each component markup requirements to use data-bs-target on non-anchor elements, and href for anchor elements only. Correct?

@jcorporation
Copy link
Contributor

I missed this breaking change. Its now all good for me, many thanks!

@jcorporation
Copy link
Contributor

Other question: where are the reference to an initialized element are stored? In previous version I can access it via triggering element e.g. el.Popover, el.Modal. In 4.1 I see no such reference anymore.

@thednp
Copy link
Owner Author

thednp commented Feb 11, 2022

That is another breaking change, you now have to do BSN.Carousel.getInstance(selector | HTMLElement). Check the updated documentation/demo ;)

@jcorporation
Copy link
Contributor

Is there a list of breaking changes?

@thednp
Copy link
Owner Author

thednp commented Feb 11, 2022

Most important changes are written in the commit linked in the initial v4.1 commit, here is the link. But I will try to find some time to compile a list of most important changes.

Update: after a second thought the introduction of Component.getInstance() method is probably the only real breaking change, all other changes are mostly consistency in terms of TypeScript or parity with the original library, which again, everything is documented with the demo/documentation. Other breaking changes are the utilities from shorter-js but should have little to no effect for any regular BSN developer.

@jcorporation
Copy link
Contributor

Works all as described!

@thednp
Copy link
Owner Author

thednp commented Feb 13, 2022

@jcorporation some questions if you don't mind:

  • does your script use dropdown / tooltip / popover? I would like to know if there is any improvement / issue in regards to automatic positioning;
  • do you think our BSN master branch is eligible for a stable release? if not, please provide info.

Thanks

@jcorporation
Copy link
Contributor

jcorporation commented Feb 13, 2022

Yes, my script uses dropdown and popovers. I have some workarounds in place. I tested now without this workarounds.

There are the same positioning problems as in the old version:

Clipped popover/dropdown (height):
image
Workaround is to set a overflow:auto and resize the popover-body, same behavior for dropdowns.

Clipped dropdowns (right):
image
Workaround is to set dynamically the dropdown-menu-end classes.

@thednp
Copy link
Owner Author

thednp commented Feb 13, 2022

The new version doesn't deal with dropdown-menu-end class anymore because of RTL support. That is something you need to set based on the context.

@jcorporation
Copy link
Contributor

jcorporation commented Feb 13, 2022

This is exactly what my workaround does. I hopped that the improved positioning feature sets the dropdown and popovers position and dimensions too prevent clipping through viewport.

@thednp
Copy link
Owner Author

thednp commented Feb 13, 2022

That is true for tooltip / popover, it will try to find the best position so it's always visible within the viewport. However dropdown is much more simple since the .dropdown-menu is contained within the .dropdown element. With other words it might be impossible to code something that is RTL compliant and automatically position the .dropdown-menu with or without the dropdown-menu-end class, believe me I tried, you know what's the reason? One invalidates the other, completely.

The improvements for dropdown are mostly related to RTL as well as more consistent positioning, but again, the dropdown-menu-end class is for you to decide where and when to use.

@jcorporation
Copy link
Contributor

Why not simply droping in the direction where is the most space? In my first screenshot the popover have more space below but it dropups.

Same with second screenshot, why the dropdown does not align to end? If I do not know the position of the dropdown button at coding time, I must determine the position at runtime and add the class accordingly.

It would be more userfriendly if bsn handle this for me. Or what usecase do I miss?

@thednp
Copy link
Owner Author

thednp commented Feb 13, 2022

That's what I have in mind. However, it already does that, exactly showing the dropdown-menu where it has the most space and reposition on window scroll/resize.

If you're using dropleft / dropright and it doesn't have space sideways, it will use either dropdown / dropup position, but it will not get from initial dropdown / dropup to dropleft / dropright as far as I can remember. Maybe here's something we could improve on.

I think you should use dropdown-menu-end class for the dropdown in the second screenshot. It would be consistent with the current scripting and also with the original library and I think shifting to the position of dropdown-menu-end without using the class would invalidate using the class in the first place.

@jcorporation
Copy link
Contributor

First screenshot: why BSN positions the popover at top and not at bottom. At bottom there are more free space. In the markup there are no direction classes, that forces a dropdup. If the screen is higher it is positioned at the bottom.

Second screenshot: I can not know the position of the dropdown element at coding time, because the button could float to the left side. The position is dependent on the screen size.

@thednp
Copy link
Owner Author

thednp commented Feb 13, 2022

Do you have a test site? I wanna have a look.

@jcorporation
Copy link
Contributor

Not for the popover, but for the dropdown element.
https://spacepirates.jcgames.de/

  • click on the search button on the top right, the search dropdown appears
  • type "wa" to search
  • close the dropdown and reopen it
  • if the dropdown does not fit at the bottom it is positioned at top even if at the bottom are more space

@thednp
Copy link
Owner Author

thednp commented Feb 13, 2022

That is "4.1.0alpha2", there have been a ton of changes since then.

@thednp
Copy link
Owner Author

thednp commented Feb 13, 2022

This is how the latest works, and how it's supposed to work

dropdown-test.mp4

@thednp
Copy link
Owner Author

thednp commented Feb 13, 2022

For some reason your test site is using the V4 version of BSN which doesn't include the changes I'm talking about.

@jcorporation
Copy link
Contributor

Sorry, I updated to the wrong version...

@jcorporation
Copy link
Contributor

jcorporation commented Feb 13, 2022

Now it should run the latest code, behavior has not changed. At the bottom there much more space than at the top. Does BSN position on top if on the bottom is not enough space independent which space is bigger?

image

@thednp
Copy link
Owner Author

thednp commented Feb 13, 2022

Your search on the site looks good to me, I've cleared the cache and the dropdown-menu is positioned where I expect it to go, except when you minimize to a mobile width and the dropdown is going into .dropdown position but doesn't adjust position.

That dropdown-menu is pretty large, if you want to use the dropstart, I would suggest giving the dropdown a max-height of about 350px and overflow-y: auto, or simply use .dropdown with the .dropdown-menu-end class.

@jcorporation
Copy link
Contributor

Initial position is as described. But if the dropdown menu does not fit in the space below it always drops up?

@thednp
Copy link
Owner Author

thednp commented Feb 13, 2022

If you want the dropdown-menu to be visible on mobile devices, makes sense to limit it's size somehow, right? There is no code to guess where an element larger than the screen should be displayed into.

@jcorporation
Copy link
Contributor

Can I force it do always dropdown and not dropup?

@thednp
Copy link
Owner Author

thednp commented Feb 13, 2022

Yes, you can disable automatic positioning, check the doc for that. Please clear the browser cache and test again, but I think you need to read careful what I wrote above and try it out. Let me know what you came out with. You might get it to work properly without changing any code, except some CSS.

@jcorporation
Copy link
Contributor

jcorporation commented Feb 13, 2022

I tested the different options, but I found no ideal way. What I wanted would be a combined dropdown and dropstart: open the menu at the left side of the button and always dropdown.

But you are right, I can overwrite this positioning with some css code.

@thednp
Copy link
Owner Author

thednp commented Feb 13, 2022

If you follow my advice, plus setting your search dropdown-menu min-width: 20rem to @media (min-width: 768) you should have an excellent outcome.

@jcorporation
Copy link
Contributor

Many thanks for your help. Now I looking for popover positioning.

@jcorporation
Copy link
Contributor

All other tings I tested: popovers, offcanvas, collapse, modals, carousel works without issues.

@thednp
Copy link
Owner Author

thednp commented Feb 13, 2022

Thank you @jcorporation I hope you managed to fix them all. As soon as I'm done with wiki updates, I'm going to push 4.1 to the cloud.

@jcorporation
Copy link
Contributor

Yes, all works as intended. It was a pleasure for me to test the new version!

@jcorporation
Copy link
Contributor

jcorporation commented Feb 13, 2022

Noticed an javascript error. It occurs only in chromium if switched to mobile device view.

image

It occurs after closing an popover. I do it manually calling the hide() method.

It is reproducible with your demo page.

thednp added a commit that referenced this issue Feb 14, 2022
* moved `tooltipTouchHandler` into the prototype to solve #430 (comment)
@jcorporation
Copy link
Contributor

I can confirm that ba090ff fixes the popover js error.

@thednp thednp closed this as completed Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community question all in for feedback help wanted need your suggestion
Projects
None yet
Development

No branches or pull requests

3 participants