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

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

Open
wants to merge 1 commit into
base: release
Choose a base branch
from

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
Collaborator

@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
Collaborator

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