Skip to content

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

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

markus-enklu wants to merge 1 commit into
releasefrom
bugfix/vine-examples

Conversation

@markus-enklu

Copy link
Copy Markdown

…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

@ShannonKalei ShannonKalei left a comment •

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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 😅

@AshleyAmazing AshleyAmazing left a comment •

Copy link
Copy Markdown

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.

This branch has not been deployed

No deployments
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