thednp / bootstrap.native

Bootstrap components build with Typescript
http://thednp.github.io/bootstrap.native/
MIT License
1.7k stars 177 forks source link

BSN 4.1 Feedback Required #430

Closed thednp closed 2 years ago

thednp commented 2 years ago

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!

lekoala commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

Here is the screnshot: image

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

jcorporation commented 2 years ago

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 commented 2 years ago

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

thednp commented 2 years ago

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 commented 2 years ago

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

jcorporation commented 2 years ago

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 commented 2 years ago

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

jcorporation commented 2 years ago

Is there a list of breaking changes?

thednp commented 2 years ago

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 commented 2 years ago

Works all as described!

thednp commented 2 years ago

@jcorporation some questions if you don't mind:

Thanks

jcorporation commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

jcorporation commented 2 years ago

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

thednp commented 2 years ago

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

thednp commented 2 years ago

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

https://user-images.githubusercontent.com/6761246/153760587-8e59ba37-029a-4634-9d2d-0e56b4e41fff.mp4

thednp commented 2 years ago

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

jcorporation commented 2 years ago

Sorry, I updated to the wrong version...

jcorporation commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

thednp commented 2 years ago

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 commented 2 years ago

Can I force it do always dropdown and not dropup?

thednp commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

jcorporation commented 2 years ago

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

thednp commented 2 years ago

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 commented 2 years ago

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

jcorporation commented 2 years ago

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.

jcorporation commented 2 years ago

I can confirm that ba090ff fixes the popover js error.