#5852 Neo Input and Neo Field

prachi00 commented 1 year ago

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
[x] Refactoring

Context

[ ] Closes #
[ ] Requires deployment <snek/rubick/worker>

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?

[x] Fill up your KSM address: [Payout]https://kodadot.xyz/dot/transfer?target=13Qx65nLd6SwdtjrRyuoEtp9CKXhF651xdHBPaXcvhwKm4N1

Community participation

[x] Are you at KodaDot Discord?

Screenshot 📸

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

Screen Shot 2023-05-08 at 12 34 09 PM

Copilot Summary

`🤖 Generated by Copilot at e1b9ba3`

No summary available (Limit exceeded: required to process 75846 tokens, but only 50000 are allowed per call)

`🤖 Generated by Copilot at 7b7ed90`

We are the NeoFields, we rise from the ashes We replace the b-fields, we bring the new flashes We are the NeoFields, we rule the forms and grids We crush the b-fields, we are the Neo kids

netlify[bot] commented 1 year ago

Deploy Preview for koda-canary ready!

Name	Link
Latest commit	331c471dd137773cff37c6867a036ef9fd773318
Latest deploy log	https://app.netlify.com/sites/koda-canary/deploys/64765187e08abe00080a625e
Deploy Preview	https://deploy-preview-5870--koda-canary.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.

reviewpad[bot] commented 1 year ago

Reviewpad Report

:warning: Warnings

Please link an issue to the pull request

vikiival commented 1 year ago

@prachi00 can you provide screenshot pls?

prachi00 commented 1 year ago

SS

@prachi00 can you provide screenshot pls?

added some

vikiival commented 1 year ago

@prachi00 can you please resolve conflicts?

prachi00 commented 1 year ago

@prachi00 can you please resolve conflicts?

done

vikiival commented 1 year ago

Agree with @roiLeo

vikiival commented 1 year ago

@kodadot/qa-guild is it possible to check this ? Seems quite a big change 😶

prachi00 commented 1 year ago

please could you check

type="is-... => variant="..."

remove message prop on NeoField

why are we removing message props? it seems it is supported by oruga https://oruga.io/components/field.html#props @roiLeo

vikiival commented 1 year ago

@prachi00 can you resolve conflicts please?

prury commented 1 year ago

@kodadot/qa-guild is it possible to check this ? Seems quite a big change 😶

will do

prachi00 commented 1 year ago

some test cases are failing after resolving conflicts, I'll fix them tomorrow

EthVlad commented 1 year ago

Hi @prachi00 is it possible to still make a little change ? I am not sure how it behaves rn but ideally have it like this for light and dark:

prachi00 commented 1 year ago

@preschian @roiLeo test cases are failing for this part when updating to neoInput, but it seems like we dont even use this function, can we remove it? can someone confirm?

preschian commented 1 year ago

test cases are failing for this part when updating to neoInput, but it seems like we dont even use this function, can we remove it? can someone confirm?

checking

preschian commented 1 year ago

test cases are failing for this part when updating to neoInput

@prachi00 I think we can use vue.extends instead. if we want to add .neo-* specific class, use vue.mixins and then forked rootClasess() from OInput

code for NeoInput.vue using mixins

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

// https://github.com/oruga-ui/oruga/blob/develop/packages/oruga/src/components/input/Input.vue
export default {
  mixins: [OInput],
  computed: {
    rootClasses() {
      return [
        'neo-input',
        this.computedClass('rootClass', 'o-ctrl-input'),
        {
          [this.computedClass('expandedClass', 'o-ctrl-input--expanded')]:
            this.expanded,
        },
      ]
    },
  },
}
</script>

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

prachi00 commented 1 year ago

test cases are failing for this part when updating to neoInput

@prachi00 I think we can use vue.extends instead. if we want to add .neo-* specific class, use vue.mixins and then forked rootClasess() from OInput

code for NeoInput.vue using mixins
<script>
import { OInput } from '@oruga-ui/oruga'

// https://github.com/oruga-ui/oruga/blob/develop/packages/oruga/src/components/input/Input.vue
export default {
  mixins: [OInput],
  computed: {
    rootClasses() {
      return [
        'neo-input',
        this.computedClass('rootClass', 'o-ctrl-input'),
        {
          [this.computedClass('expandedClass', 'o-ctrl-input--expanded')]:
            this.expanded,
        },
      ]
    },
  },
}
</script>

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

thank you for this, works fine now and tests are clearing as well

EthVlad commented 1 year ago

@prachi00 checked latest commit, found just few things: General:

set left & right padding 16px
remove box shadow

Dark mode:

change border color from #CCC to #999999

And I have a question about top&bottom padding and line-height (8px as is now looks ok as well, since there is 16px font and height of the field is 48px. Line height is probably the reason there is 8px and not 12px) How exactly is the line-height set as of now? I can see that rn we are using 1.5 rem?

prury commented 1 year ago

Transfer:

Insert Address should begin with Capital I
Add button needs alignment

Identity

E-mail or Email instead of email

Collection

Deployed version(Lacking some description at the bottom of it and also the char count):
beta.kodadot.xyz:
deployed version: lacking char count
beta.kodadot.xyz:

Create NFT

Char counters not showing
Field alignment doesn't match:
item description when active(deployed version):
beta.kodadot.xyz:

EthVlad commented 1 year ago

Deployed version(Lacking some description at the bottom of it and also the char count):

In case there is any desciptive text lets have it like this. It also done this here: #5939

Also please change the error color to match our red which is #FF5757 (k-red) Either than that error messages can stay as is (under textfield left) until further change.

prachi00 commented 1 year ago

I think the margin bottom is 12px default by oruga, we can change it though, should we change to 8?

prachi00 commented 1 year ago

looks like this now

PS, havent made the alignment fix yet as it is using <b-select, and it would be fixed when migrating <b-select to neoselect

@VladoTheBoi @prury

prury commented 1 year ago

looks like this now

PS, havent made the alignment fix yet as it is using <b-select, and it would be fixed when migrating <b-select to neoselect

@VladoTheBoi @prury

cool! is it the same case for this one?

also:

This happens when i input a number on it:

https://github.com/kodadot/nft-gallery/assets/36627808/b2891908-40bd-40eb-a670-3a79df59e22b

Plus button gets deactivated when there is a form verification pending( it doesn't happen in beta.kodadot.xyz):

https://github.com/kodadot/nft-gallery/assets/36627808/789bc054-bde2-43d9-bd4b-97dfef6a44f3

EthVlad commented 1 year ago

I think the margin bottom is 12px default by oruga, we can change it though, should we change to 8?

Which margin do you mean exactly?

EthVlad commented 1 year ago

Deployed version(Lacking some description at the bottom of it and also the char count):

In case there is any desciptive text lets have it like this. It also done this here: #5939

Also please change the error color to match our red which is #FF5757 (k-red) Either than that error messages can stay as is (under textfield left) until further change.

@prachi00 I posted this, not sure you've seen that. Still several things need change:

Title is Bold
title is 12px from textfield
if subtitle present, its 4px under title and 12px from textfield
charcount is 4px under textfield, alligned right
padding within a textfield is 16px on sides and 12px Top&Bottom
change error color to #FF5757 (k-red)

vikiival commented 1 year ago

So few things:

@VladoTheBoi what is missing please open a follow-up issue
@prachi00 please resolve conflicts and you have some comments from @prury, would love to merge this week

prachi00 commented 1 year ago

So few things:

@VladoTheBoi what is missing please open a follow-up issue

@prachi00 please resolve conflicts and you have some comments from @prury, would love to merge this week

yeah we need to merge, conflicts keep popping up everyday 😢

prachi00 commented 1 year ago

@prury I dont understand the issue with 1st video? is it that usd value is not getting updated? I see it getting updated though so not sure

As for the 2nd one, it seems theres an icon that isnt showing when form is invalid and its taking up all the space @roiLeo @preschian I see that the :before isnt getting applied here to icon from oruga and that is why it isnt showing up currently, would you have any idea why the icon isnt showing up?

for now, I have hidden this class so that the danger icon doesnt show up, we can solve it in another issue as this PR is getting huge

preschian commented 1 year ago

it seems theres an icon that isnt showing when form is invalid and its taking up all the space

there is no icon named alert-circle on fa https://fontawesome.com/icons/alert-circle?s=solid. could you change to something else?

prury commented 1 year ago

@prachi00 Watched my video again and its not really showing what i was trying to record, the whole site was jumping/shifting/updating when i typed a number on the amount field, but seems to be fixed now.

The add button on the transfer page is still lacking alignment tho.

prachi00 commented 1 year ago

I'll fix the add button alignment in the followup issue with other css fixes

prachi00 commented 1 year ago

it seems theres an icon that isnt showing when form is invalid and its taking up all the space

there is no icon named alert-circle on fa https://fontawesome.com/icons/alert-circle?s=solid. could you change to something else?

I didnt add it though, this icon was added by oruga, I'll look into replacing it if we can

prachi00 commented 1 year ago

fixed the conflicts again, can we merge now, I'll do the rest of styling changes in followup issue @vikiival

vikiival commented 1 year ago

@prachi00 can you please open the follow-up issue with changes that needs to be done?

prachi00 commented 1 year ago

@prachi00 can you please open the follow-up issue with changes that needs to be done?

done

vikiival commented 1 year ago

So 🥺

roiLeo commented 1 year ago

So 🥺

Screenshot 2023-05-30 at 15-13-21 #5852 Neo Input and Neo Field by prachi00 · Pull Request #5870 · kodadot_nft-gallery

knock-doorknob

prachi00 commented 1 year ago

resolved conflicts again, can be merged now

codeclimate[bot] commented 1 year ago

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

View more on Code Climate.

prachi00 commented 1 year ago

Also, e2e test cases are failing on main now, I have fixed some of them in this pr by removing it as I think it doesnt exist anymore, I hope that is correct, more are failing but I want to confirm that removing them is the right way to fix it? let me know if I need to revert those changes if we need them, i'll do it 🫣 CC @preschian @vikiival @roiLeo

prachi00 commented 1 year ago

Also, e2e test cases are failing on main now, I have fixed some of them in this pr by removing it as I think it doesnt exist anymore, I hope that is correct, more are failing but I want to confirm that removing them is the right way to fix it? let me know if I need to revert those changes if we need them, i'll do it 🫣 CC @preschian @vikiival @roiLeo

PS can we merge this pr now please, as the test cases failing are not related to this anyway

vikiival commented 1 year ago

I hope that is correct,

So that means you don't propagate them?

prachi00 commented 1 year ago

I hope that is correct,

So that means you don't propagate them?

what do you mean propagate?

vikiival commented 1 year ago

what do you mean propagate?

What was the reason to remove lines from the tests. And how to bring the back

prachi00 commented 1 year ago

what do you mean propagate?

What was the reason to remove lines from the tests. And how to bring the back

because they did not exist and thats why they failed, do we even want them back?

kodadot / nft-gallery