Galley item hide properties/offers #4460

daiagi · 2022-12-08T13:53:24Z

Thank you for your contribution to the KodaDot NFT gallery.

👇 _ Let's make a quick check before the contribution.

PR Type

Bugfix
Feature
Refactoring

Context

Closes Hide Properties when they are empty #4438
Requires deployment <>

Before submitting pull request, please make sure:

My contribution builds clean without any errors or warnings
I've merged recent default branch -- main and I've no conflicts
I've tried to respect high code quality standards
I've didn't break any original functionality
I've posted a screenshot of demonstrated change in this PR

Optional

I've tested it at </bsx/collection>
I've tested PR on mobile
I've written unit tests 🧪
I've found edge cases

Had issue bounty label?

Fill up your KSM address: Payout

Community participation

Are you at KodaDot Discord?

Screenshot 📸

My fix has changed something on UI; a screenshot is best to understand changes for others.

…lley-item-hide-properties/offers

netlify · 2022-12-08T13:53:29Z

✅ Deploy Preview for koda-nuxt ready!

Name	Link
🔨 Latest commit	`54c4c26`
🔍 Latest deploy log	https://app.netlify.com/sites/koda-nuxt/deploys/639c87e79f7c440008a27af9
😎 Deploy Preview	https://deploy-preview-4460--koda-nuxt.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

roiLeo

Hey!
Let's start with something simple:

disable o-tab-item when there is no MAKE_OFFER event in nft.events
show tooltip for rmrk if prefix rmrk
show tooltip for disabled when no event

components/rmrk/Gallery/Gallery.vue

daiagi · 2022-12-08T15:37:27Z

Hey! Let's start with something simple:

* disable `o-tab-item` when there is no `MAKE_OFFER` event in `nft.events`

* show tooltip for rmrk if prefix rmrk

* show tooltip for disabled when no event

the tooltip is the heart of where it gets messy
in order to have tooltip on the tab header i need to provide o-tab-item with a header slot, agree?
because the header slot is a child of a disabled button generated by o-tab-item, it doesn't receive hover events
from there the road is set for all the css you see
maybe it's hackish, and i would love to have it simpler, but i see no alternative so far.

the real issue starts when extracting this into a component and needing to put o-tab-item's children where they belong

if you will permit it, i can push a working version where the required behavior is working, but it's not extracted into a component
but same as you, I don't like it, it should be in a component

should i uncomment the slot so you can see live what is happening?

btw, I assume i'm missing some detail regarding the use of slots that f** it up, I'm hoping you may be able to tell what it is with a glance

roiLeo · 2022-12-08T16:04:16Z

the tooltip is the heart of where it gets messy in order to have tooltip on the tab header i need to provide o-tab-item with a header slot, agree?

Yes, I would rather have directly change GalleryItemTabsPanel to include <template #header>

all the css you see maybe it's hackish, and i would love to have it simpler, but i see no alternative so far.

please, let's try to avoid as much as possible css hack 🥺

the real issue starts when extracting this into a component and needing to put o-tab-item's children where they belong

Are you refering to your GalleryTab component? IMO, it's not needed
Btw, oruga-ui shouldn't be used in kodadot app, it was originally planned for histoire components.

daiagi · 2022-12-08T17:08:11Z

@roiLeo
so i pushed a simplified version to illustrate the issue
there is no from the main branch except the addition of a simple <template #header>

   <o-tab-item value="0" class="py-5">
      <template #header> Properties </template>
      <o-table
        v-if="nftMetadata?.attributes.length"
        :data="nftMetadata?.attributes"
        hoverable>
        <o-table-column v-slot="props" field="value" label="Trait">
          {{ props.row.value }}
        </o-table-column>
        <o-table-column
          v-slot="props"
          field="trait_type"
          :label="$t('tabs.tabProperties.section')">
          {{ props.row.trait_type }}
        </o-table-column>
      </o-table>
    </o-tab-item>

still, the header tab is empty. when the page loads it shows at first, but then disappears after 1-2 seconds
seems to only happen in RMRK

daiagi · 2022-12-08T17:38:06Z

Found it!
it happens when o-tab-item recieves no children
solution is to pass empty div as v-else

    <o-tab-item value="0" class="py-5">
      <template #header> Properties </template>
      <o-table
        v-if="nftMetadata?.attributes.length"
        :data="nftMetadata?.attributes"
        hoverable>
        <o-table-column v-slot="props" field="value" label="Trait">
          {{ props.row.value }}
        </o-table-column>
        <o-table-column
          v-slot="props"
          field="trait_type"
          :label="$t('tabs.tabProperties.section')">
          {{ props.row.trait_type }}
        </o-table-column>
      </o-table>
      <div v-else></div>
    </o-tab-item>

roiLeo · 2022-12-08T17:50:23Z

Sorry, I didn't knew tooltip wasn't working with disabled tab

Let me know if you find something, might be good to open an issue in oruga repo?

daiagi · 2022-12-08T17:52:48Z

Sorry, I didn't knew tooltip wasn't working with disabled tab

Let me know if you find something, might be good to open an issue in oruga repo?

it's ok. I got it already
i'll submit working PR tommorow and we'll continue improving from that
Thank you 😄

libs/ui/src/components/TooltipTab/TooltipTab.vue

daiagi · 2022-12-09T10:12:44Z

@roiLeo
this assignment is proving much more challenging then it appears on the surface... 😫
I've switch to use oruga-ui tool tip and it working nicely in histoire

but now not working in the app
by not working i mean not showing on hover

some more details that may help you help me...

if i put always prop like this in the histoire component

      <o-tooltip
        v-if="disabled"
        append-to-body
        always
        class="disabled-tab-tooltip"
        position="top"
        :label="disabledTooltip"
        @click.native.stop>
        {{ label }}
      </o-tooltip>

then when the page renders it looks

and then, when hovering over offers tab the tooltip "finds" it's position

any ideas?

daiagi · 2022-12-13T09:45:18Z

Ready to go isn't it?

You might take a look at #4480 (comment)

I see... anything i can do to help resolve that?

roiLeo · 2022-12-13T09:48:42Z

Ready to go isn't it?

You might take a look at #4480 (comment)

I see... anything i can do to help resolve that?

Could you test when setting z-index: 100; on main element if it's working as excepted and if there is no side effects?

…lley-item-hide-properties/offers

daiagi · 2022-12-13T10:42:34Z

@roiLeo the only side effect I've found so far is this:

on a related issue... how do we deal with responsive font-size (to prevent tooltip from overflowing the screen on mobile)
I expermineted and found this to work

    .o-tip__content {
        font-size: max(min(2vw, 0.9rem), 0.45rem);

but maybe this is not the preferred method?

roiLeo · 2022-12-13T12:24:25Z

@roiLeo the only side effect I've found so far is this:

can be fixed with a background-color

on a related issue... how do we deal with responsive font-size (to prevent tooltip from overflowing the screen on mobile) I expermineted and found this to work
    .o-tip__content {
        font-size: max(min(2vw, 0.9rem), 0.45rem);
but maybe this is not the preferred method?

It won't really be displayed on mobile, but yes it seems to me to be a good solution as long as you don't use px

daiagi · 2022-12-13T13:01:09Z

@roiLeo the only side effect I've found so far is this:
* can be fixed with a background-color

mmm it actually has background already

roiLeo · 2022-12-13T13:04:25Z

mmm it actually has background already

works for me:

daiagi · 2022-12-13T13:10:04Z

it depends whether or not the gallery item in question has the "More of this collection" section
this one has, and the tooltip works
https://deploy-preview-4460--koda-nuxt.netlify.app/rmrk/gallery/10177217-0a24c7876a892acb79-RADTOADZ-RADTOADZ-0000000000000103?redesign=true

the one @exezbcz mentioned doesn't, and then depending on the screen size the footer image either interferes or not
anyway it unreliable

setting main z-index to 100, solves this problem but introduces the tooltip on mobile problem I mentioned above
which maybe is not really an issue, since there is no tooltip on hover in mobile anyway

…lley-item-hide-properties/offers

daiagi · 2022-12-15T10:10:10Z

@roiLeo is there anything more i need to do here?
do you want me to set z-index:100 as part of this PR?

roiLeo · 2022-12-15T13:26:11Z

@roiLeo is there anything more i need to do here? do you want me to set z-index:100 as part of this PR?

Would be top if you could fix https://deploy-preview-4460--koda-nuxt.netlify.app/rmrk/gallery/15527227-B02B8226FEE3EAA06D-ART-BRIDGE-0000000000000001?redesign=true hover not visible.

Next, wait for a 2nd approval and it's good to go.

…lley-item-hide-properties/offers

…om/daiagi/nft-gallery into galley-item-hide-properties/offers

daiagi · 2022-12-16T05:02:00Z

Would be top if you could fix https://deploy-preview-4460--koda-nuxt.netlify.app/rmrk/gallery/15527227-B02B8226FEE3EAA06D-ART-BRIDGE-0000000000000001?redesign=true hover not visible.

Done ✔️

libs/ui/src/components/DisablableTab/DisablableTab.vue

yangwao · 2022-12-16T13:50:00Z

pay 60 usd

let's resolve conflicts @daiagi and merge it! :)

yangwao · 2022-12-16T13:50:12Z

😍 Perfect, I’ve sent the payout
💵 $60 @ 26.64 USD/KSM ~ 2.252 $KSM
🧗 EfmnRhHaQqfT3phm4cUCHCU3gFVDoSPR1U9WXzMRQBMqZ4L
🔗 0x8c938dfb0435482e592b2b853515271702f2ecb0137d265184abfafe01620d30

🪅 Let’s grab another issue and get rewarded!
🪄 github.com/kodadot/nft-gallery/issues

…lley-item-hide-properties/offers

codeclimate · 2022-12-16T15:00:19Z

Code Climate has analyzed commit 54c4c26 and detected 0 issues on this pull request.

View more on Code Climate.

daiagi · 2022-12-16T15:01:17Z

let's resolve conflicts @daiagi and merge it! :)

Done @yangwao

daiagi added 6 commits December 8, 2022 16:41

intitial commit

2e3e8ec

Merge branch 'main' of https://github.com/kodadot/nft-gallery into ga…

b3a89ef

…lley-item-hide-properties/offers

merge leftover

0f0cc11

extract component

4cc1fc3

translations

036bf78

Merge branch 'main' of https://github.com/kodadot/nft-gallery into ga…

272c352

…lley-item-hide-properties/offers

daiagi requested a review from a team as a code owner December 8, 2022 13:53

daiagi requested review from prachi00 and removed request for a team December 8, 2022 13:53

daiagi marked this pull request as draft December 8, 2022 13:53

daiagi added 2 commits December 8, 2022 21:06

fix translation

b540ffe

remove leftover experiment

17e9855

roiLeo self-requested a review December 8, 2022 14:21

daiagi added 2 commits December 8, 2022 21:25

remove unused props var

a66bda1

remove leftovers

bae0197

roiLeo reviewed Dec 8, 2022

View reviewed changes

components/rmrk/Gallery/Gallery.vue Outdated Show resolved Hide resolved

simplified version to illustarte bug

59cbc51

extract to histoire component

ef2179f

roiLeo reviewed Dec 9, 2022

View reviewed changes

libs/ui/src/components/TooltipTab/TooltipTab.vue Outdated Show resolved Hide resolved

daiagi added 3 commits December 9, 2022 15:50

TooltipTab story

050a6ad

css adjustements

18253c7

switch tooltip from beufy to oruga

7a4b219

Merge branch 'main' of https://github.com/kodadot/nft-gallery into ga…

b8cb353

…lley-item-hide-properties/offers

daiagi added 2 commits December 13, 2022 20:12

Merge branch 'main' of https://github.com/kodadot/nft-gallery into ga…

1991804

…lley-item-hide-properties/offers

Merge branch 'main' into galley-item-hide-properties/offers

99d2c30

daiagi added 5 commits December 16, 2022 11:52

💄 set main z-index:100

a0fed1e

💄 responsive tooltip size

5117cca

✅ make room for tooltip on mobile

6f0a4ae

Merge branch 'main' of https://github.com/kodadot/nft-gallery into ga…

a54b9d0

…lley-item-hide-properties/offers

Merge branch 'galley-item-hide-properties/offers' of https://github.c…

f30f863

…om/daiagi/nft-gallery into galley-item-hide-properties/offers

prachi00 approved these changes Dec 16, 2022

View reviewed changes

roiLeo added S-code-lgtm-✅ code review guild has reviewed this PR and it's code is approved S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked labels Dec 16, 2022

preschian reviewed Dec 16, 2022

View reviewed changes

libs/ui/src/components/DisablableTab/DisablableTab.vue Show resolved Hide resolved

yangwao added paid pull-request has been paid S-changes-requested-🤞 PR is almost good to go, just some fine tunning labels Dec 16, 2022

Merge branch 'main' of https://github.com/kodadot/nft-gallery into ga…

54c4c26

…lley-item-hide-properties/offers

yangwao merged commit 15ef9f8 into kodadot:main Dec 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Galley item hide properties/offers #4460

Galley item hide properties/offers #4460

daiagi commented Dec 8, 2022 •

edited

netlify bot commented Dec 8, 2022 •

edited

roiLeo left a comment

daiagi commented Dec 8, 2022 •

edited

roiLeo commented Dec 8, 2022

daiagi commented Dec 8, 2022 •

edited

daiagi commented Dec 8, 2022

roiLeo commented Dec 8, 2022

daiagi commented Dec 8, 2022

daiagi commented Dec 9, 2022 •

edited

daiagi commented Dec 13, 2022

roiLeo commented Dec 13, 2022 •

edited

daiagi commented Dec 13, 2022

roiLeo commented Dec 13, 2022

daiagi commented Dec 13, 2022

roiLeo commented Dec 13, 2022

daiagi commented Dec 13, 2022 •

edited

daiagi commented Dec 15, 2022

roiLeo commented Dec 15, 2022

daiagi commented Dec 16, 2022

yangwao commented Dec 16, 2022 •

edited

yangwao commented Dec 16, 2022

codeclimate bot commented Dec 16, 2022

daiagi commented Dec 16, 2022 •

edited

Galley item hide properties/offers #4460

Galley item hide properties/offers #4460

Conversation

daiagi commented Dec 8, 2022 • edited

PR Type

Context

Before submitting pull request, please make sure:

Optional

Had issue bounty label?

Community participation

Screenshot 📸

netlify bot commented Dec 8, 2022 • edited

✅ Deploy Preview for koda-nuxt ready!

roiLeo left a comment

Choose a reason for hiding this comment

daiagi commented Dec 8, 2022 • edited

roiLeo commented Dec 8, 2022

daiagi commented Dec 8, 2022 • edited

daiagi commented Dec 8, 2022

roiLeo commented Dec 8, 2022

daiagi commented Dec 8, 2022

daiagi commented Dec 9, 2022 • edited

daiagi commented Dec 13, 2022

roiLeo commented Dec 13, 2022 • edited

daiagi commented Dec 13, 2022

roiLeo commented Dec 13, 2022

daiagi commented Dec 13, 2022

roiLeo commented Dec 13, 2022

daiagi commented Dec 13, 2022 • edited

daiagi commented Dec 15, 2022

roiLeo commented Dec 15, 2022

daiagi commented Dec 16, 2022

yangwao commented Dec 16, 2022 • edited

yangwao commented Dec 16, 2022

codeclimate bot commented Dec 16, 2022

daiagi commented Dec 16, 2022 • edited

daiagi commented Dec 8, 2022 •

edited

netlify bot commented Dec 8, 2022 •

edited

daiagi commented Dec 8, 2022 •

edited

daiagi commented Dec 8, 2022 •

edited

daiagi commented Dec 9, 2022 •

edited

roiLeo commented Dec 13, 2022 •

edited

daiagi commented Dec 13, 2022 •

edited

yangwao commented Dec 16, 2022 •

edited

daiagi commented Dec 16, 2022 •

edited