Serialize Props with no Type about uxpin-merge-tools HOT 20 CLOSED

But we could be more clever and parse the value from presets to figure it out. But my thinking was, that they also might have not specified default values/preset file.

Good points. I vote for not trying to be clever at first (either a String or just an error message in properties).

from uxpin-merge-tools.

I do have one todo (already discussed with Hiro and @bdebicki)
I do use components from our design system and would be better to ask frontend specialist to do required visual changes

Sounds good to me. Would @bdebicki be able to help with these visual changes?

I focused on any type and unfortunately missed part about "If a prop doesn't have a type" but i do have question in here: how do you understand this part? Can you share example component?

The main concern is handling the any type, as this is very commonly used. By the way, are there similar any types in Flow/TypeScript that we will need to handle?

I do assume that you think about property which is mentioned in preset like 0-default.jsx but NOT is not defined in propTypes = {} but i want to be sure that we talk about this case as it sound a bit like dirty hacking to include them

Yes - this is what I was thinking. It's possible that people can pass props into their component (via the preset file for example), but not have these props defined in ProTypes/Flow/TypeScript. It is definitely not good practice, but I have seen it happen before. Could you experiment with this, and maybe if we detect anything we can serialise it in the CLI with the any type, and then handle it in the same way in the frontend. Let's investigate and see how difficult that would be, before spending too much time on it.

from uxpin-merge-tools.

Hey @bdebicki

Ad. 1
that's a good point!
I think we can still do better than displaying disabled properties. i did draw some other solution how we can handle this case. this is a pattern we using in other situations and it meets the conditions we would like to keep:

Thanks for all your suggestions and insights, I appreciate all the thought that you have put into this. But let's move ahead with the disabled field with a tooltip message as discussed. We can always re-visit the issue in future if we change our minds.

Ad. 2
i wanted to use this copy from the beginning but our tooltip has some issues with links inside it. property isClickable does not work and the tooltip disappears when you go mouse out from the trigger. If the link is required here I'm gonna do a fix in the tooltip component

Yeah, if we could try to get the link in, then that would be really great. If we can't, then that's ok, but let's give it a go and see. It does sound like a tricky issue though.

from uxpin-merge-tools.

@bdebicki / @matthewchigira - let's try to merge and deploy the support for props with no type with the current way #201 (comment)

Sorry, I think I've dragged the attention into the different issue("props only defined in presets file"). let's come back to it in the future.

I think an input doesn't have to be disabled for now. "disabled form input" idea was more for props only defined in preset file to respect our pattern.

So, as far as I know, we can fix message (@matthewchigira - maybe you can leave a correct messaging for it in PR?) and deploy once the PRs are approved.

from uxpin-merge-tools.

i have a good news about the tooltip bug. I almost finished fix for it. i have to handle 2 more cases and write some tests. i should finish it on monday.
this give us possibility to put the link into a message for any type field

from uxpin-merge-tools.

Just to reiterate, with a PropType.any, you'll see the following in experiment mode in the properties (this component has no other properties):

from uxpin-merge-tools.

When no type is present, assume it's type

Do you have thoughts on how to do this? Were you thinking inferring from the preset value?

If we wanted to hold on thinking thru how to assume the type, I wonder if we could display the property name in the UI and a message where a form field would usually be: "A specific property type is required. Please see our docs on property mapping."

from uxpin-merge-tools.

Do you have thoughts on how to do this? Were you thinking inferring from the preset value?

I was initially thinking of just assuming a String value and providing a free text box. But we could be more clever and parse the value from presets to figure it out. But my thinking was, that they also might have not specified default values/preset file.

If we wanted to hold on thinking thru how to assume the type, I wonder if we could display the property name in the UI and a message where a form field would usually be: "A specific property type is required. Please see our docs on property mapping."

I do like this idea, as it at least shows that we that have noticed prop is there, but then we leave it up to the customer to fix.

from uxpin-merge-tools.

except of lot of digging in it was not complicated to add support for "any" type i believe. It didnt even require changes in merge-cli and it seems that after release change should be visible for all components which were already synced (as props with "any" type were not cut off; just not visible in UI)
code is in here but its not PR yet: https://github.com/UXPin/uxpin-app/compare/merge-prop-type-any?expand=1

1. I do have one todo (already discussed with Hiro and @bdebicki)
   I do use components from our design system and would be better to ask frontend specialist to do required visual changes

2. question to @matthewchigira
   I focused on any type and unfortunately missed part about "If a prop doesn't have a type" but i do have question in here: how do you understand this part? Can you share example component?
   I do assume that you think about property which is mentioned in preset like 0-default.jsx but NOT is not defined in propTypes = {} but i want to be sure that we talk about this case as it sound a bit like dirty hacking to include them

EDIT
i checked if it works on staging env + on experimental mode with this command:
./node_modules/.bin/uxpin-merge --disable-tunneling --uxpin-domain=uxpin-rails-merge-prop--yleani.uxpinstage.com
i put it in here just for history as review app will be probably removed soon

from uxpin-merge-tools.

Yes - this is what I was thinking. It's possible that people can pass props into their component (via the preset file for example), but not have these props defined in ProTypes/Flow/TypeScript. It is definitely not good practice, but I have seen it happen before. Could you experiment with this, and maybe if we detect anything we can serialise it in the CLI with the any type, and then handle it in the same way in the frontend. Let's investigate and see how difficult that would be, before spending too much time on it.

We use https://github.com/reactjs/react-docgen to serialize props in. I would stick with what react-docgen returns on CLI side. I wonder if it's easy to detect on our editor though? 🤔

from uxpin-merge-tools.

Hi @mzah could you go ahead and create a PR, and then test and confirm that a component with the any type works in all three scenarios: using PropTypes, using Flow and using TypeScript? If it's a UI change, then there is no reason why it should work for all three, but we will need to confirm this.

from uxpin-merge-tools.

kk

1. i made PR ( https://github.com/UXPin/uxpin-app/pull/9154/files )
   Rob did a first look, i fixed first comment BUT:
   bartek still do work on visual side i assume so: work is in progress
2. I did check proptypes, flow and TS and in all cases it worked fine as expected
3. if you want me to continue with "type on defined" case (with preset as source of properties) we would need agreement
   a) i found where i can merge preset with definition on merge-cli side
   b) but hiro idea (do not hack CLI; do hacks on UI side) also sounds tempting. I didnt look on this case from this side yet. should i?

from uxpin-merge-tools.

I've done UI part and it looks like this:

What i had to add is to handle the warning message for code mode - we have an alternative view for the properties panel:

I've changed the message to be more user-friendly.
From what I see some tests in the frontend part are failing and I think it will be the last thing that has to be done for this task.

here are 2 additional prs related to this change:
https://github.com/UXPin/uxpin-assets/pull/3731
https://github.com/UXPin/uxpin-design-system/pull/306

from uxpin-merge-tools.

tests are fixed now

from uxpin-merge-tools.

Yes - this is what I was thinking. It's possible that people can pass props into their component (via the preset file for example), but not have these props defined in ProTypes/Flow/TypeScript. It is definitely not good practice, but I have seen it happen before. Could you experiment with this, and maybe if we detect anything we can serialise it in the CLI with the any type, and then handle it in the same way in the frontend. Let's investigate and see how difficult that would be, before spending too much time on it.

We use https://github.com/reactjs/react-docgen to serialize props in. I would stick with what react-docgen returns on CLI side. I wonder if it's easy to detect on our editor though? 🤔

i ve check and we don't pass properties that are not defined in prop types in metadata and any other way that give us a possibility to handle it in the properties panel. i think we can't do this without work on cli.
i even think we shouldn't think about handling this. our requirement is clear - in the properties panel we display only properties that are defined so if a user would like to display property on the interface it should be defined. handling cases like this may lead to unexpected behaviors

from uxpin-merge-tools.

Thank you @bdebicki for the investigation!

i’ve check and we don’t pass properties that are not defined in prop types in metadata and any other way that give us a possibility to handle it in the properties panel. i think we can’t do this without work on cli.

I believe we pass this information actually. When you use the component on our editor, it gets presets data from our api. And we use it to set initial value. So, whenever users change props on property panel, it’s overriding the initial values defined in preset file.
And what we are doing for presets file from CLI side is, really just serializing the props. If you do npx uxpin-merge dump, you'll see the serialized presets props.

You'll see something like this. (I added propOnlyExistsInPrest in preset file). You can also click 👇 to see full output of dump

          "presets": [
            {
              "elements": {
                "button": {
                  "name": "Button",
                  "props": {
                    "children": "Let's Merge!",
                    "icon": {
                      "uxpinPresetElementId": "button2"
                    },
                    "mode": "filled",
                    "propOnlyExistsInPrest": "test",
                    "size": "m",
                    "stretched": true,
                    "type": "primary"
                  }
                },
                "button2": {
                  "name": "Icon",
                  "props": {
                    "icon": "TickerSvg",
                    "size": "s"
                  }
                }
              },
              "name": "default",
              "rootId": "button"
            }

You are using @uxpin/merge-cli version: 2.7.2

Browserslist: caniuse-lite is outdated. Please run next command `yarn upgrade`
{
  "categorizedComponents": [
    {
      "components": [],
      "name": "General"
    },
    {
      "components": [
        {
          "documentation": {
            "examples": [
              {
                "code": "<Button\n    stretched={true}\n    type=\"error\"\n    mode=\"filled\"\n    icon={<Icon size=\"s\" icon=\"TickerSvg\" />}\n    size=\"s\"\n    >Merge!</Button>"
              }
            ]
          },
          "info": {
            "dirPath": "src/components/Button",
            "documentation": {
              "path": "src/components/Button/Button.md"
            },
            "implementation": {
              "framework": "reactjs",
              "lang": "javascript",
              "path": "src/components/Button/Button.js"
            },
            "presets": [
              {
                "path": "src/components/Button/presets/0-default.jsx"
              }
            ]
          },
          "name": "Button",
          "presets": [
            {
              "elements": {
                "button": {
                  "name": "Button",
                  "props": {
                    "children": "Let's Merge!",
                    "icon": {
                      "uxpinPresetElementId": "button2"
                    },
                    "mode": "filled",
                    "propOnlyExistsInPrest": "test",
                    "size": "m",
                    "stretched": true,
                    "type": "primary"
                  }
                },
                "button2": {
                  "name": "Icon",
                  "props": {
                    "icon": "TickerSvg",
                    "size": "s"
                  }
                }
              },
              "name": "default",
              "rootId": "button"
            }
          ],
          "properties": [
            {
              "description": "",
              "isRequired": false,
              "name": "onClick",
              "type": {
                "name": "func",
                "structure": {}
              }
            },
            {
              "defaultValue": {
                "value": false
              },
              "description": "",
              "isRequired": false,
              "name": "disabled",
              "type": {
                "name": "boolean",
                "structure": {}
              }
            },
            {
              "defaultValue": {
                "value": "filled"
              },
              "description": "",
              "isRequired": false,
              "name": "mode",
              "type": {
                "name": "union",
                "structure": {
                  "elements": [
                    {
                      "name": "literal",
                      "structure": {
                        "value": "filled"
                      }
                    },
                    {
                      "name": "literal",
                      "structure": {
                        "value": "ghost"
                      }
                    },
                    {
                      "name": "literal",
                      "structure": {
                        "value": "minimal"
                      }
                    },
                    {
                      "name": "literal",
                      "structure": {
                        "value": "flat"
                      }
                    }
                  ]
                }
              }
            },
            {
              "description": "",
              "hidden": true,
              "isRequired": false,
              "name": "title",
              "type": {
                "name": "string",
                "structure": {}
              }
            },
            {
              "description": "",
              "hidden": true,
              "isRequired": false,
              "name": "background",
              "type": {
                "name": "string",
                "structure": {}
              }
            },
            {
              "customName": "Label",
              "description": "",
              "isRequired": false,
              "name": "children",
              "type": {
                "name": "string",
                "structure": {}
              }
            },
            {
              "description": "",
              "isRequired": false,
              "name": "icon",
              "type": {
                "name": "node",
                "structure": {}
              }
            },
            {
              "defaultValue": {
                "value": "left"
              },
              "description": "",
              "isRequired": false,
              "name": "iconDirection",
              "type": {
                "name": "union",
                "structure": {
                  "elements": [
                    {
                      "name": "literal",
                      "structure": {
                        "value": "left"
                      }
                    },
                    {
                      "name": "literal",
                      "structure": {
                        "value": "right"
                      }
                    }
                  ]
                }
              }
            },
            {
              "defaultValue": {
                "value": "m"
              },
              "description": "",
              "isRequired": false,
              "name": "size",
              "type": {
                "name": "union",
                "structure": {
                  "elements": [
                    {
                      "name": "literal",
                      "structure": {
                        "value": "xs"
                      }
                    },
                    {
                      "name": "literal",
                      "structure": {
                        "value": "s"
                      }
                    },
                    {
                      "name": "literal",
                      "structure": {
                        "value": "m"
                      }
                    },
                    {
                      "name": "literal",
                      "structure": {
                        "value": "l"
                      }
                    },
                    {
                      "name": "literal",
                      "structure": {
                        "value": "xl"
                      }
                    },
                    {
                      "name": "literal",
                      "structure": {
                        "value": "xxl"
                      }
                    },
                    {
                      "name": "literal",
                      "structure": {
                        "value": "xxxl"
                      }
                    }
                  ]
                }
              }
            },
            {
              "defaultValue": {
                "value": true
              },
              "description": "",
              "isRequired": false,
              "name": "stretched",
              "type": {
                "name": "boolean",
                "structure": {}
              }
            }
          ],
          "wrappers": []
        }
      ],
      "name": "Form"
    }
  ],
  "name": "UXPin Merge Boilerplate",
  "vcs": {
    "branchName": "master",
    "commitHash": "391c939b5eb3b1f49cc836ded37226f79ca3d096",
    "paths": {
      "configPath": "/Users/naohiro/xenon/uxpin/uxpin-merge-boilerplate/uxpin.config.js",
      "projectRoot": "/Users/naohiro/xenon/uxpin/uxpin-merge-boilerplate"
    }
  }
}

i even think we shouldn’t think about handling this. our requirement is clear - in the properties panel we display only properties that are defined so if a user would like to display property on the interface it should be defined. handling cases like this may lead to unexpected behaviors

I totally agree with this. But message would be helpful, I think. Something like disabled input filed with tooltip saying "This property is not editable. If you would like to, please reach out to your dev team and add this to propstype"

I would try to deploy what we worked on so far and come back to this issue as a further improvement in the future :)

@matthewchigira - what do you think?

from uxpin-merge-tools.

i think we should solve this case in a different way than in the properties panel.
maybe during pushing in console display a warning with information about these properties?

there're a few reasons why i think the properties panel is not the place we should inform users about this.

• we have a pattern that displays only properties that the user can edit or can be run in a specific context - these properties are disabled for a full time for the user. this is a global pattern for uxpin's properties panel. we don't display properties that can't be used in the element - this approach based on our user research
• based on feedback from users, that was "properties panel is illegible" we did a change where we remove all properties with types that user can't use (because we didn't support it) + we add jsdoc comment handling to hide some of the properties if they are not valuable for designers (like refs, test handlers, etc). that was a part of the "designer experience" feature.
• we should remember properties panel is a part of designers, pms and other users that will be designing in uxpin and this information is more for devs instead of designers
• we have a property like uxpid that we don't display it in uxpin what about properties like this? what if we would like to add more like this? we probably have to do some filtering for properties like this.
• right now we clearly define from where properties panel controls are taken after the change we going to have an additional place (even if it is a disabled one)
• controls like this will destruct users flow through the properties panel and confusing them

from uxpin-merge-tools.

Hi @bdebicki @mzah @naohiro-t

Thank you all for your comments and suggestions, this has been a good discussion, and I've been thinking it over for a few days.

I totally agree with this. But message would be helpful, I think. Something like disabled input filed with tooltip saying "This property is not editable. If you would like to, please reach out to your dev team and add this to propstype"
I would try to deploy what we worked on so far and come back to this issue as a further improvement in the future :)

Agreed. Happy to make it non-editable, this would reduce the risk of unexpected behaviour. But I definitely want to display the field in the properties panel, just so that we can show the user that we know about it. This is one of the main stumbling blocks when people try Merge for the first time, they get frustrated that their props are showing and give up.

i think we should solve this case in a different way than in the properties panel.
maybe during pushing in console display a warning with information about these properties?
there're a few reasons why i think the properties panel is not the place we should inform users about this.

I can understand where you are coming from, and I think you make some great points. But as for an onboarding stumbling block, this is a huge blocker, because people ignore the CLI warning messages (or sometime don't see them because of CI) and then get frustrated with the lack of information in the UXPin app.

So let's do this:

1. Make the field disabled
2. Change the text to "This property is not editable because a specific property type is required. Please see our docs on property mapping."

We can always come back to the issue in future if we find that this solution could be improved.

from uxpin-merge-tools.

Ad. 1

people ignore the CLI warning messages (or sometime don't see them because of CI) and then get frustrated with the lack of information in the UXPin app.

that's a good point!
I think we can still do better than displaying disabled properties. i did draw some other solution how we can handle this case. this is a pattern we using in other situations and it meets the conditions we would like to keep:

• display information about unavailable properties because of lack of type
• sticks into a rule of cleanness of controls in the properties panel

you can check here interactive prototype with tooltip -> https://preview.uxpin.com/c0f53978867370b47cb4cd7fd60c88f00586ba5f#/pages/137177294?mode=cvhidm

Ad. 2
i wanted to use this copy from the beginning but our tooltip has some issues with links inside it. property isClickable does not work and the tooltip disappears when you go mouse out from the trigger. If the link is required here I'm gonna do a fix in the tooltip component

from uxpin-merge-tools.

Closed by #9154

from uxpin-merge-tools.