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

Updating the vertical position of the zoom-in text box to have its arrow point to the center of the zoom-in button #3453

Merged
merged 9 commits into from
Jan 23, 2024

Conversation

srihariKrishnaswamy
Copy link
Collaborator

@srihariKrishnaswamy srihariKrishnaswamy commented Jan 4, 2024

Resolves #3378

In this PR I align the arrow of the text box that prompts the user to zoom in in the tutorial to the center of the zoom in button. I did this by setting the top of the box to be where the center should be, and adding another parameter in OnboardingStates that translated up the box by 50% of it's height. I also had to replace the fadeInDown/Left/Right animation settings for each arrowed text box with a simple fadeIn animation, since the transformation component of the fadeInDown/Left/Right was overwriting the base transform needed to position each box properly.

Before/After screenshots (if applicable)

Before:
image

After:
image

Testing instructions
  1. Choose a language
  2. Step through the tutorial until you get to the point where the text box pops up telling the user to zoom in and observe if it points to the center of the button.
Things to check before submitting the PR
  • I've written a descriptive PR title.
  • I've added/updated comments for large or confusing blocks of code.
  • I've included before/after screenshots above.

@srihariKrishnaswamy
Copy link
Collaborator Author

I did have one question, would you like me to do this for the zoom-out text-box as well? I was going to but then realized I probably shouldn't do anything not on the issue explicitly.

@misaugstad
Copy link
Member

@srihariKrishnaswamy is this just for that one dialog that we took a screenshot of for the issue, or for all of the dialogs that are pointing to an element of the interface? Just making sure bc it should do the latter!

@srihariKrishnaswamy
Copy link
Collaborator Author

Yeah I only did the former in the PR, sounds good I'll do it for the other ones as well!

…onboarding states to fadeIn since translate in animation was messing with text box positioning
@srihariKrishnaswamy
Copy link
Collaborator Author

srihariKrishnaswamy commented Jan 5, 2024

Alright, I went ahead and changed it for all of them! The thing that was messing with the raw transform: translateX/Y(-50%) was that the original fadeInDown/Left/Right animations already had a transform component, so that was getting overwritten. What I newly did was to change all the fadeInDown/Left/Right animations to a simple fadeIn so there are no translation discrepancies. Let me know if you'd like me to resolve it differently though!

@misaugstad
Copy link
Member

The thing that was messing with the raw transform: translateX/Y(-50%) was that the original fadeInDown/Left/Right animations already had a transform component, so that was getting overwritten.

Great sleuthing! Can't wait to get a chance to take a look and test!

@misaugstad
Copy link
Member

This looks SOOO much better, thank you!! Now that you've got some experience with this, I'm wondering if you can try to make some more small adjustments to make things really consistent..? I have a few screenshots with suggested improvements. But I don't think that you need to fix all of them. I imagine that some of them will be easy to fix, and some will be hard. For the ones that end up being surprisingly difficult, don't spend hours and hours on them! But if you can't fix something, please include what you learned in a comment on the PR so that we know for the future!

  1. For some reason, it looks like the location for the zoom buttons is slightly lower in the Chinese version..? It looks consistently in the center in every other language, but for some reason in Chinese it looks to be below center...
    Screenshot from 2024-01-11 10-58-36
    Screenshot from 2024-01-11 10-57-57
    Screenshot from 2024-01-11 11-10-41
  2. It looks like these dialogs aren't vertically centered with the mini map. There are three different ones.
    Screenshot from 2024-01-11 10-51-29
  3. We seem to be inconsistent with how far away our little arrows are from the buttons that we are pointing to. I would love to have these being consistent! I think that the distance we have in the first screenshot would be ideal!
    Screenshot from 2024-01-11 10-50-29
    Screenshot from 2024-01-11 10-50-59
    Screenshot from 2024-01-11 10-49-49
  4. And how hard would it be to horizontally center with the navigation message it's above here? I'm assuming that we can base it off of reading the width of the nav message?
    Screenshot from 2024-01-11 10-49-49
    Screenshot from 2024-01-11 10-56-45

@srihariKrishnaswamy
Copy link
Collaborator Author

Alright, I fixed all the above except for number 4! The reason for that being the bottom fainter text box and the text box with the arrow are not direct siblings, so working in CSS to position the arrowed text box relative to the bottom one would be a bit difficult & would require me to change how everything else is positioned as well I think. Additionally, in order to fix the first issue you mentioned, I couldn't find the reason why the chinese specifically was causing it to go lower, so I ended up just translating the top up a little bit for those specific arrows you pointed out whenever the language is set to chinese. Let me know if you'd like me to handle this differently though.

Thanks!

@misaugstad
Copy link
Member

fixed all the above except for number 4! The reason for that being the bottom fainter text box and the text box with the arrow are not direct siblings, so working in CSS to position the arrowed text box relative to the bottom one would be a bit difficult & would require me to change how everything else is positioned as well I think

Gotcha, no problem!

I ended up just translating the top up a little bit for those specific arrows you pointed out whenever the language is set to chinese. Let me know if you'd like me to handle this differently though.

I think at that point, I'd rather just have them be a few pixels off for the Chinese version. It's still quite close to being centered, so I'd rather not add in that extra odd check!

@srihariKrishnaswamy
Copy link
Collaborator Author

Sounds good, I just pushed up a version with those last changes reneged!

Copy link
Member

@misaugstad misaugstad left a comment

Choose a reason for hiding this comment

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

I just removed some CSS you forgot to remove and fixed a bug I found when finishing the tutorial in German (not related to your code). Love what you did, it looks much better now!

@misaugstad misaugstad merged commit e6d786a into develop Jan 23, 2024
@misaugstad misaugstad deleted the 3378-adjust-instruction-locations-dynamically branch January 23, 2024 18:14
@misaugstad misaugstad mentioned this pull request Jan 31, 2024
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.

Tutorial: dynamically adjust instruction locations based on language / text length
2 participants