Skip to content

Comments

Changed visible=false to visible=true in Vine examples and added a li…#26

Open
markus-enklu wants to merge 1 commit intoreleasefrom
bugfix/vine-examples
Open

Changed visible=false to visible=true in Vine examples and added a li…#26
markus-enklu wants to merge 1 commit intoreleasefrom
bugfix/vine-examples

Conversation

@markus-enklu
Copy link

…nk to the VineML page to make it immediately clear that there is more information on VineML available

…nk to the VineML page to make it immediately clear that there is more information on VineML available
Copy link
Contributor

@ShannonKalei ShannonKalei left a comment

Choose a reason for hiding this comment

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

Another one of those "who thought this was a good idea?" type examples 😅 But ty for fixing 🙏 Though now that I've actually looked at this, there's a lot more wrong with this example:

  • font.size should be fontSize
  • the position is set to +10 meters in the z direction, so you likely wouldn't even be able to see it
  • visibility defaults to true, so maybe we should just remove it entirely?
  • width also seems like a weird addition since this example is just 1 word 🤔
  • Also, I'm pretty sure <Caption> is an old version of <Text>. They are the same under the hood and the vines are still backwards compatible, but maybe we should swap this to <Text> since that's likely a clearer term than caption?

@ShannonKalei
Copy link
Contributor

Here is what I propose we change it to:

  <Text
    label = 'Hello World'
    fontSize = 100
    fontColor = '#FFFF00'
    width = 1000.5
    alignment = 'MidCenter'
    position = (0, 0.5, 0.5)
  />
  • Change Caption to Text
  • Change label to have 2 words so the width attribute seems more relevant
  • Change font.size to fontSize
  • Add fontColor because that seems like a common user need
  • Add alignment because that also seems like a common user need
  • Change position from (0, 0, 10) to (0, 0.5, 0.5) so you can actually see the text 😅

Copy link

@AshleyAmazing AshleyAmazing left a comment

Choose a reason for hiding this comment

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

width=1000.5?

What a strange number, I wish we had more context for this number, is it based on screen size? The rest looks good to me. Text width defaults to 1500 but .5 of what a pixel? an em? a pt? I know it's a float but I wish it made more sense for text.

Also I agree with Shannon and recommend you add in those changes as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants