Product switcher on mobile #424
Conversation
This reverts commit 8a06a3c.
parostatkiem-zz
requested review from
antiheld,
dariadomagala-sap,
hardl,
jesusreal,
JohannesDoberer,
kwiatekus,
marynaKhromova,
maxmarkus,
pekura and
y-kkamil
as code owners
| toggleDropdownState(name) { | ||
| toggleDropdownState(name, event) { | ||
| if (event) | ||
| event.stopPropagation(); |
There was a problem hiding this comment.
it looks like prettier did not kick in --> it should either be enclosed within { } or in the same line
| data-cy="desktop-product-switcher" | ||
| ></button> | ||
| </div> | ||
| {#if !isMobile} |
There was a problem hiding this comment.
shouldn't this be moved to the top (line 6)? - the markup above is not being displayed on mobile anyway.. or am i just wrong? 🤔
core/src/navigation/TopNav.html
Outdated
| @@ -217,14 +237,27 @@ | |||
| } | |||
| }, | |||
| methods: { | |||
| openMobileProductSwitcher(event) { | |||
| if (event) | |||
| event.stopPropagation(); | |||
|
please take a look at my comment above:
other than that: i think it is ok |
| </span> | ||
| <span class="y-full-width-list__subtitle"> | ||
| Subtitle | ||
| <!-- TODO: add proper subtitle --> |
There was a problem hiding this comment.
I would suggest adding it now, or removing this Subtitle text for now :)
There was a problem hiding this comment.
So.. should I add it to ProductSwitcher config?
There was a problem hiding this comment.
I think so. Please check it with @hardl
There was a problem hiding this comment.
Anyway, I vote for a separate task here. If we introduced a new config variable, the docu would need to be updated and it'd be nice to use it in desktop PS also...
…roduct-switcher-on-mobile # Conflicts: # core/package-lock.json
core/src/navigation/TopNav.html
Outdated
| <ProductSwitcher | ||
| bind:dropDownStates | ||
| on:toggleDropdownState="toggleDropdownState(event.name)" | ||
| on:setProductSwitcherToDesktop="set({isProductSwitcherMobile: false})" |
There was a problem hiding this comment.
This line is not needed anymore with the new approach.
| {/each} | ||
| {#if isProductSwitcherAvailable} | ||
| <li> | ||
| <a class="fd-menu__item" on:click="openMobileProductSwitcher(event)" data-cy="mobile-product-switcher"> |
There was a problem hiding this comment.
Now we could just leave for the click event toggleDropdownState('productSwitcherPopover'), nothing else is done in the openMobileProductSwitcher function anymore.
There was a problem hiding this comment.
You are wrong, the event propagation is stopped. That's why it doesn't work when I put toggleDropdownState here. But I did this change in ProductSwitcher.html where stopping propagation wasn't needed :)
y-kkamil
dismissed
their stale review
Due to serious changes after my approval, I will take a look once again
* code cleanup & add comments * Change mobile switcher button look * Update fiori-fundamentals * Improve mobile list styling * Fix fd-shellbar--end text-align * Update Cypress * Add e2e tests * Remove unused code * Simplify PS management by adding separate mobile instance * Remove "subtitle" label
Description
My Products label in popover menu is not aligned with its icon. This bug will be fixed in another PR.
The version of Product Switcher depends on which link was clicked to open it. This means it's dependent on the fact if shellbar is collapsed or not, not on a viewport size directly. With this approach, the Product Switcher won't be damaged by any further changes in shellbar's behaviour and the situation with having desktop menu and mobile Product Switcher (and opposite) is impossible.
Changes proposed in this pull request:
Related issue(s)