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

Feature NeoSelect #6200

Closed
wants to merge 39 commits into from
Closed

Conversation

brano-hozza
Copy link
Contributor

@brano-hozza brano-hozza commented Jun 10, 2023

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

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

Optional

  • I've tested it at </ksm/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

@brano-hozza brano-hozza requested a review from a team as a code owner June 10, 2023 09:16
@brano-hozza brano-hozza requested review from vikiival and Jarsen136 and removed request for a team June 10, 2023 09:16
@netlify
Copy link

netlify bot commented Jun 10, 2023

Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit 26c7163
🔍 Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/649985a09d7f1a000846cf52
😎 Deploy Preview https://deploy-preview-6200--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 Jun 10, 2023

AI-Generated Summary: This pull request includes two patches:

  1. The first patch adds basic CSS and transforms the CollectionSelect component to use the NeoSelect component. It also creates a new NeoSelect.scss file for styling purposes. A new translation key is added for the English locale file for the "select collection" and "select collection description" strings.
  2. The second patch changes the icon to a normal icon in the NeoSelect.vue component, and adds "oruga" to the list of recognized words in the .vscode/settings.json file.

@reviewpad reviewpad bot added medium Pull request is medium waiting-for-review labels Jun 10, 2023
@vikiival
Copy link
Member

Currently hacking @ ETHPrague 🥺

Screenshot 2023-06-10 at 11 23 36

@brano-hozza
Copy link
Contributor Author

Also I’ve noticed that I forgot to change other references to select component 😆 my Mac is dead so will fix later

@brano-hozza brano-hozza reopened this Jun 10, 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.

any idea why test failing?

  • error 500 on /series-insight
    Screenshot 2023-06-12 at 16-52-27 KodaDot - NFT Market Explorer

libs/ui/src/components/NeoSelect/NeoSelect.vue Outdated Show resolved Hide resolved
components/settings/Interface.vue Outdated Show resolved Hide resolved
Co-authored-by: roiLeo <medina.leo42@gmail.com>
@prury
Copy link
Member

prury commented Jun 12, 2023

So far:

  • Missing the amount of NFTs i have inside a collection: (Create NFT page)

image

  • Clicking at the arrow wont open the dropdown: (General)

image

I'm still checking it tho

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

prury commented Jun 12, 2023

Not sure if its related, but only this deployed version is allowing me to select those other options no matter what network i'm in (its disabled for both beta an canary):

image

@brano-hozza
Copy link
Contributor Author

@prury not sure about the extra options in balance input, but the problem with unclickable arrow is problem in oruga library as there is no handler for the custom icons

@brano-hozza brano-hozza requested a review from roiLeo June 12, 2023 16:54
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.

✅ code lgtm & wfm

Screenshot 2023-06-13 at 08-59-50 KodaDot Low fees and low carbon minting KodaDot - NFT Market Explorer

the problem with unclickable arrow is problem in oruga library as there is no handler for the custom icons

I will be following oruga issue

</template>

<script lang="ts" setup>
import { OField, OSelect } from '@oruga-ui/oruga'
<script setup>
Copy link
Member

Choose a reason for hiding this comment

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

is there any concern if we using mixins instead here? should be easier to migrate b-select, and the test should be ok

<script>
import { OSelect } from '@oruga-ui/oruga'

export default {
  mixins: [OSelect],
  computed: {
    rootClasses() {
        return [
          'neo-select',
            this.computedClass('rootClass', 'o-ctrl-sel'),
            { [this.computedClass('expandedClass', 'o-ctrl-sel--expanded')]: this.expanded },
        ]
    },
  },
}
</script>

<style lang="scss">
@import './NeoSelect.scss';
</style>

Copy link
Member

Choose a reason for hiding this comment

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

Clicking at the arrow wont open the dropdown: (General)

I don't have this issue with extending OSelect components

Copy link
Contributor

Choose a reason for hiding this comment

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

are selectClasses imported too?
I'm ok with that but will #6200 (comment) still work?

Copy link
Contributor

Choose a reason for hiding this comment

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

Clicking at the arrow wont open the dropdown: (General)

I don't have this issue with extending OSelect components

Check darkmode custom arrow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But did u use custom arrow element? because their native arrow doesn't support dark mode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay but still oruga select doesnt have options prop so you would need to manually add options everywhere, but my solution provides prop to set options with provided array and arrow from mixin doesnt work in dark mode

Copy link
Member

@preschian preschian Jun 13, 2023

Choose a reason for hiding this comment

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

Okay but still oruga select doesnt have options prop so you would need to manually add options everywhere

yes, maybe this is Oruga principle to go that way https://github.com/oruga-ui/oruga/blob/develop/.github/CONTRIBUTING.md#2-lightweight-and-simple for now

arrow from mixin doesnt work in dark mode

you can add this CSS to fix the icon in dark mode. by using this, we can click the arrow to open the dropdown

.dark-mode .neo-select .o-sel-arrow {
  background-image: url("data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' height='1em' viewBox='0 0 512 512'%3E%3C!--! Font Awesome Free 6.4.0 by @fontawesome - https://fontawesome.com License - https://fontawesome.com/license (Commercial License) Copyright 2023 Fonticons, Inc. --%3E%3Cstyle%3Esvg%7Bfill:%23ffffff%7D%3C/style%3E%3Cpath d='M233.4 406.6c12.5 12.5 32.8 12.5 45.3 0l192-192c12.5-12.5 12.5-32.8 0-45.3s-32.8-12.5-45.3 0L256 338.7 86.6 169.4c-12.5-12.5-32.8-12.5-45.3 0s-12.5 32.8 0 45.3l192 192z'/%3E%3C/svg%3E");
}

.light-mode .neo-select .o-sel-arrow {
  background-image: url("data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' height='1em' viewBox='0 0 512 512'%3E%3C!--! Font Awesome Free 6.4.0 by @fontawesome - https://fontawesome.com License - https://fontawesome.com/license (Commercial License) Copyright 2023 Fonticons, Inc. --%3E%3Cstyle%3Esvg%7Bfill:%23000000%7D%3C/style%3E%3Cpath d='M233.4 406.6c12.5 12.5 32.8 12.5 45.3 0l192-192c12.5-12.5 12.5-32.8 0-45.3s-32.8-12.5-45.3 0L256 338.7 86.6 169.4c-12.5-12.5-32.8-12.5-45.3 0s-12.5 32.8 0 45.3l192 192z'/%3E%3C/svg%3E");
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay solution with arrow is nice, but the argument about oruga way of doing things… I dont know, I tried to reduce the boilerplate with my solution 😅

Copy link
Member

Choose a reason for hiding this comment

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

imo, if the component already exists on Oruga, just use their default behaviour first by extending their component. to make it easier, because our goal is to migrate from buefy to oruga first

if we want to introduce another behaviour or even a better solution. it's good, but we can do it after migration or put it in the other custom component. in this case, we can create separate PR and then create another component <NeoSelectOption /> with your solution

another example is that we still have b-navbar that doesnt have an oruga component. we can copy buefy implementation, or maybe create our own component

Copy link
Contributor

Choose a reason for hiding this comment

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

imo, if the component already exists on Oruga, just use their default behaviour first by extending their component. to make it easier, because our goal is to migrate from buefy to oruga first

Yes we first introduce breaking changes because we missed important wrapping class (#6197)
Let's first focus on migration and then we can improve some components.

another example is that we still have b-navbar that doesnt have an oruga component. we can copy buefy implementation, or maybe create our own component

https://github.com/oruga-ui/theme-bulma#buefy-users "Soon" ahah but I think custom Bulma navbar is the way.

@roiLeo roiLeo mentioned this pull request Jun 14, 2023
8 tasks
@yangwao
Copy link
Member

yangwao commented Jun 14, 2023

image

let's fix conflicts, probably wont' go to upcoming production release which is planned in today/tomorrow as I sense select on lot of places and has lot of cases.

@reviewpad
Copy link
Contributor

reviewpad bot commented Jun 19, 2023

Reviewpad Report

⚠️ Warnings

  • Please link an issue to the pull request

@vikiival
Copy link
Member

So what's the current state?

@roiLeo
Copy link
Contributor

roiLeo commented Jun 26, 2023

So what's the current state?

let it roll 🎢

@vikiival
Copy link
Member

Screenshot 2023-06-26 at 14 30 16

@codeclimate
Copy link

codeclimate bot commented Jun 26, 2023

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

View more on Code Climate.

@brano-hozza
Copy link
Contributor Author

brano-hozza commented Jun 26, 2023

@vikiival I will open a new PR with changes sugested by @preschian to use mixins. It would be much cleaner to find errors in this refactor as I need to rollback multiple changes. I can get to it later today or tommorow

@vikiival
Copy link
Member

Understandable ^-^

@vikiival vikiival closed this Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
large Pull request is large S-blocked-✋ S-changes-requested-🤞 PR is almost good to go, just some fine tunning S-code-lgtm-✅ code review guild has reviewed this PR and it's code is approved waiting-for-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants