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

#6223 double stroke issues #6263

Merged
merged 12 commits into from
Jul 11, 2023
Merged

#6223 double stroke issues #6263

merged 12 commits into from
Jul 11, 2023

Conversation

prachi00
Copy link
Member

Thank you for your contribution to the KodaDot - One Stop Shop for Polkadot NFTs.

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

PR Type

  • Bugfix
  • Feature
  • Refactoring

Needs QA check

  • @kodadot/qa-guild please review

Context

Did your issue had any of the "$" label on it?

Screenshot 📸

  • My fix has changed UI
Screen Shot 2023-06-17 at 3 45 41 PM Screen Shot 2023-06-17 at 3 46 09 PM

Copilot Summary

@prachi00 prachi00 requested a review from a team as a code owner June 17, 2023 22:46
@prachi00 prachi00 requested review from roiLeo and vikiival and removed request for a team June 17, 2023 22:46
@kodabot
Copy link
Collaborator

kodabot commented Jun 17, 2023

WARNING @prachi00 PR for issue #6223 which isn't assigned to you. Please be warned that this PR may get rejected if there's another assignee for issue #6223

@netlify
Copy link

netlify bot commented Jun 17, 2023

Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit a97fd31
🔍 Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/64ac603c19ab9900084ab9c2
😎 Deploy Preview https://deploy-preview-6263--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 configuration.

@reviewpad
Copy link
Contributor

reviewpad bot commented Jun 17, 2023

AI-Generated Summary: This pull request addresses the double stroke issues in #6223 by making changes to components/explore/ExploreGrid.vue, components/gallery/GalleryItemActionSlides.vue, components/massmint/Massmint.vue, and components/navbar/CreateDropdown.vue files. It adds a new class "middle-icon" to the grid size button in ExploreGrid.vue, changes border styles to have "!important" in GalleryItemActionSlides.vue and Massmint.vue, and modifies the code structure in CreateDropdown.vue to separate dropdown menu items based on the "chain" value.

@reviewpad reviewpad bot added small Pull request is small waiting-for-review labels Jun 17, 2023
Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

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

This look like a hotfix, Imo something wrong with NeoTab scss config

@prachi00
Copy link
Member Author

This look like a hotfix, Imo something wrong with NeoTab scss config

what do you mean wrong?

@prachi00
Copy link
Member Author

@roiLeo does something needs to be changed here?

@roiLeo
Copy link
Contributor

roiLeo commented Jun 20, 2023

@roiLeo does something needs to be changed here?

I don't really like this solution, we'll always have this problem check profile page buttons for example

Capture d’écran 2023-06-20 à 8 55 29 AM

@prachi00
Copy link
Member Author

@roiLeo does something needs to be changed here?

I don't really like this solution, we'll always have this problem check profile page buttons for example

Capture d’écran 2023-06-20 à 8 55 29 AM

I dont think there is any other way to fix it though other than manual css?

@roiLeo
Copy link
Contributor

roiLeo commented Jun 21, 2023

I dont think there is any other way to fix it though other than manual css?

it should work using default oruga css, check out doc example of field with addons.

@prachi00
Copy link
Member Author

Screen Shot 2023-06-21 at 10 08 54 PM Screen Shot 2023-06-21 at 10 09 07 PM @roiLeo even in oruga fields, there is double border

Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

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

I would try someting different, create a css rule where all element inside a NeoField with addons should have border-right removed but :not(:last-child)

@prury
Copy link
Member

prury commented Jun 22, 2023

@prachi00, i know i said in the issue that some other PR fixed it, but i've checked again and is still happening:
image

@prury prury added the S-changes-requested-🤞 PR is almost good to go, just some fine tunning label Jun 22, 2023
@prachi00
Copy link
Member Author

@prachi00, i know i said in the issue that some other PR fixed it, but i've checked again and is still happening: image

which chain is this? I dont see it

@prury
Copy link
Member

prury commented Jun 23, 2023

@prachi00, i know i said in the issue that some other PR fixed it, but i've checked again and is still happening: image

which chain is this? I dont see it

RMRK2:
https://deploy-preview-6263--koda-canary.netlify.app/ksm/gallery/18429103-7EA1DCF47E98A25067-3MAX-UNIEDITION_1-00000001

@prachi00
Copy link
Member Author

Screen Shot 2023-06-24 at 2 31 23 PM @prury weird I dont see it, I am on chrome, and you are on chrome too?

@prury
Copy link
Member

prury commented Jun 25, 2023

Screen Shot 2023-06-24 at 2 31 23 PM @prury weird I dont see it, I am on chrome, and you are on chrome too?

I'm on Firefox, you got it, it's a Firefox only issue, tested on Chrome and it won't happen.

@prachi00
Copy link
Member Author

@prury
I cant reproduce this even in firefox
Screen Shot 2023-06-27 at 11 54 41 AM

@roiLeo can we merge this one for now? We can create a different issue to track firefox issue
Also, PS giving border left to fields might not work as in some cases we have border bottom as well that is double stroked and in some cases its not the fields but tabs

Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

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

I don't see any difference with beta, why would we merge it?

@prachi00
Copy link
Member Author

Screen Shot 2023-06-28 at 7 38 34 PM in beta there is double stroke

@prachi00
Copy link
Member Author

prachi00 commented Jul 2, 2023

maybe we can close this pr if its not relevant anymore
@exezbcz @roiLeo

Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

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

For multiple buttons wrapped in NeoField I would see something like:

.o-field--addons > :not(:first-child):not(:last-child) button,
.o-field--addons > button:not(:first-child):not(:last-child) {
  border-right: 0;
  border-left: 0;
}

and I think we can do something similar for MassMint tabs

Why do you have some changes in CreateDropdown component?

prachi00 and others added 2 commits July 5, 2023 15:29
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@prachi00
Copy link
Member Author

prachi00 commented Jul 5, 2023

For multiple buttons wrapped in NeoField I would see something like:

.o-field--addons > :not(:first-child):not(:last-child) button,
.o-field--addons > button:not(:first-child):not(:last-child) {
  border-right: 0;
  border-left: 0;
}

and I think we can do something similar for MassMint tabs

Why do you have some changes in CreateDropdown component?

for massmints, cant find a common class as it an <a tag
dropdown changes are gone after resolving conflicts

Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

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

for massmints, cant find a common class as it an <a tag dropdown changes are gone after resolving conflicts

Alright, noted.

code lgtm

@roiLeo roiLeo added S-code-lgtm-✅ code review guild has reviewed this PR and it's code is approved and removed S-changes-requested-🤞 PR is almost good to go, just some fine tunning labels Jul 6, 2023
@prury prury added the S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked label Jul 6, 2023
@prury
Copy link
Member

prury commented Jul 6, 2023

only double stroke remaining was the one on profile, but let's leave it for profile redesign, works for me.

@prachi00
Copy link
Member Author

can we merge this pls

@codeclimate
Copy link

codeclimate bot commented Jul 10, 2023

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

View more on Code Climate.

@vikiival
Copy link
Member

pay 13 usd

@vikiival vikiival merged commit 675ccfd into main Jul 11, 2023
@vikiival vikiival deleted the feat-double-stroke branch July 11, 2023 17:10
@yangwao
Copy link
Member

yangwao commented Jul 11, 2023

😍 Perfect, I’ve sent the payout
💵 $13 @ 5.15 USD/DOT ~ 2.524 $DOT
🧗 13Qx65nLd6SwdtjrRyuoEtp9CKXhF651xdHBPaXcvhwKm4N1
🔗 0x0bc14f00dfadf159899946f804442dd4283186ac4a9703d8326465af8cb55241

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

@yangwao yangwao added the paid pull-request has been paid label Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
paid pull-request has been paid 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 small Pull request is small waiting-for-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Double stroke visual issue
6 participants