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.

Sign up for GitHub

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

Jump to bottom

βž– remove b-tooltip component #5856

Merged
merged 10 commits into from
May 18, 2023
Merged

βž– remove b-tooltip component #5856

merged 10 commits into from
May 18, 2023

Conversation

roiLeo
Copy link
Contributor

@roiLeo roiLeo commented May 3, 2023 •
edited

One tooltip still need to be migrated but is multilined

PR Type

  • Refactoring

Context

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

Screenshot πŸ“Έ

Capture d’écran 2023-05-12 aΜ€ 4 01 50 PM

Screenshot 2023-05-03 at 15-37-28 KodaDot - NFT Market Explorer

Copilot Summary

πŸ€– Generated by Copilot at 150d681

Refactored the minting UI and code to use Vue 3 composition API and remove unused components. Deleted Tooltip.vue and moved Minting.vue logic to DropUpload.vue. Simplified the minting settings and improved the code quality.

πŸ€– Generated by Copilot at 150d681

Minting.vue is no more, we moved it to DropUpload.vue
Refactoring the code with Vue 3, we don't need the Tooltip
Simplifying the settings UI, we improve the user experience
We are the masters of minting, we resist the code complexity

@roiLeo roiLeo requested a review from a team as a code owner May 3, 2023 13:39
@roiLeo roiLeo requested review from vikiival and daiagi and removed request for a team May 3, 2023 13:39
@netlify
Copy link

netlify bot commented May 3, 2023 •
edited

βœ… Deploy Preview for koda-canary ready!

Name Link
πŸ”¨ Latest commit 99c09d4
πŸ” Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/6464878c29ff0e0008946b5e
😎 Deploy Preview https://deploy-preview-5856--koda-canary.netlify.app
πŸ“± Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@reviewpad
Copy link
Contributor

reviewpad bot commented May 3, 2023

AI-Generated Summary: This pull request removes the b-tooltip component and its associated imports from the DropUpload.vue, SimpleMint.vue, and Minting.vue components. The Tooltip.vue component file itself is also deleted. Other changes involve updating the structure of Minting.vue and removing some unnecessary template elements. This results in a total of 109 deletions and 24 insertions across 4 files.

@reviewpad reviewpad bot added medium Pull request is medium waiting-for-review labels May 3, 2023
@reviewpad
Copy link
Contributor

reviewpad bot commented May 3, 2023

Reviewpad Report

⚠️ Warnings

  • Please link an issue to the pull request

@roiLeo roiLeo mentioned this pull request May 3, 2023
Closed
37 tasks
@vikiival
Copy link
Member

vikiival commented May 3, 2023

Sir Oruga tooltip exist https://oruga.io/components/Tooltip.html

vikiival
vikiival requested changes May 3, 2023
View reviewed changes
Copy link
Member

@vikiival vikiival left a comment

Choose a reason for hiding this comment

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

We have tooltip in oruga

@roiLeo
Copy link
Contributor Author

roiLeo commented May 3, 2023

We have tooltip in oruga

Sure thing, won't fix "Settings" Tooltip as it's bad UX.
Feel free to close it.

@vikiival
Copy link
Member

vikiival commented May 3, 2023

Feel free to close it.

I would rather opt in for oruga tooltip, and do UX stuff with redesign

@roiLeo
Copy link
Contributor Author

roiLeo commented May 3, 2023

I would rather opt in for oruga tooltip, and do UX stuff with redesign

Capture d’écran 2023-05-03 aΜ€ 5 52 18 PM

It's here but it need some (breaking?) changes to work like our current spec.

@vikiival
Copy link
Member

vikiival commented May 3, 2023

cc @exezbcz

@exezbcz
Copy link
Member

exezbcz commented May 3, 2023

@vikiival what is needed πŸ‘€

@roiLeo
Copy link
Contributor Author

roiLeo commented May 4, 2023

nvm forgot to replace <b-tooltip /> 🀦 , draft for now

@roiLeo roiLeo marked this pull request as draft May 4, 2023 08:18
@roiLeo roiLeo marked this pull request as ready for review May 12, 2023 14:02
vikiival
vikiival approved these changes May 15, 2023
View reviewed changes
Copy link
Member

@vikiival vikiival left a comment

Choose a reason for hiding this comment

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

Otherwise oki

@reviewpad reviewpad bot added large Pull request is large and removed medium Pull request is medium labels May 15, 2023
daiagi
daiagi requested changes May 15, 2023
View reviewed changes
Copy link
Contributor

@daiagi daiagi left a comment •
edited

Choose a reason for hiding this comment

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

please try to remove append-to-body wherever possible to avoid tooltip shown not above content

components/bsx/Offer/OfferTable.vue Show resolved Hide resolved
components/common/IdentityForm.vue Outdated Show resolved Hide resolved
components/rmrk/Gallery/EmotionList.vue Outdated Show resolved Hide resolved
components/rmrk/Gallery/History.vue Outdated Show resolved Hide resolved
components/rmrk/Profile/ProfileDetail.vue Outdated Show resolved Hide resolved
daiagi
daiagi reviewed May 16, 2023
View reviewed changes
components/gallery/GalleryItemAction/GalleryItemActionType/GalleryItemBuy.vue Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

all the tooltips under GalleryItemActionType need append-to-body otherwise they are hidden

components/navbar/CreateDropdown.vue
@@ -7,48 +7,48 @@
</div>
</template>
<b-dropdown-item has-link>
<b-tooltip
position="is-left"
<NeoTooltip
Copy link
Contributor

Choose a reason for hiding this comment

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

tooltip overflows

image

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made a change to have it multiline but I find it change the UI, should I keep it multiline?

Copy link
Contributor

@daiagi daiagi May 17, 2023 •
edited

Choose a reason for hiding this comment

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

Either that or change max width of tooltip to large value to avoid it overflowing

I would opt for increasing max width

@daiagi
Copy link
Contributor

daiagi commented May 16, 2023

text under tooltip is too large, compare with beta

image
image

@roiLeo
Copy link
Contributor Author

roiLeo commented May 16, 2023

text under tooltip is too large, compare with beta

It's like that on beta...
Capture d’écran 2023-05-16 aΜ€ 5 06 11 PM

@daiagi
Copy link
Contributor

daiagi commented May 16, 2023

text under tooltip is too large, compare with beta

It's like that on beta... Capture d’écran 2023-05-16 aΜ€ 5 06 11 PM

i'm talking about the "confirm 1/2" text
beta:
image

canary:
image

@roiLeo
Copy link
Contributor Author

roiLeo commented May 17, 2023

i'm talking about the "confirm 1/2" text beta

#5971 will be fixed by #6013

@daiagi daiagi added the S-code-lgtm-βœ… code review guild has reviewed this PR and it's code is approved label May 17, 2023
@roiLeo @vikiival
Update components/shared/form/BasicSwitch.vue
99c09d4 
Co-authored-by: Viki Val <viktorko99@gmail.com>
@codeclimate
Copy link

codeclimate bot commented May 17, 2023

Code Climate has analyzed commit 99c09d4 and detected 0 issues on this pull request.

View more on Code Climate.

@yangwao yangwao merged commit 3442a84 into kodadot:main May 18, 2023
19 checks passed
@yangwao yangwao mentioned this pull request May 18, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daiagi daiagi daiagi approved these changes

@vikiival vikiival vikiival approved these changes

Assignees
No one assigned
Labels
large Pull request is large S-code-lgtm-βœ… code review guild has reviewed this PR and it's code is approved waiting-for-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@roiLeo @vikiival @exezbcz @daiagi @yangwao