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

Feat: 1692 tooltips #1774

Merged
merged 29 commits into from
Jul 18, 2023
Merged

Feat: 1692 tooltips #1774

merged 29 commits into from
Jul 18, 2023

Conversation

BCerki
Copy link
Collaborator

@BCerki BCerki commented Jul 6, 2023

Copy link
Contributor

@Sepehr-Sobhani Sepehr-Sobhani left a comment

Choose a reason for hiding this comment

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

looks great 🤩 I'm just not sure about creating snapshots from form pages; we need to change them every time we change a single line in those form pages. what do you think?!

Copy link
Contributor

Choose a reason for hiding this comment

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

I love the idea of keeping all tooltip text in one place.

@BCerki
Copy link
Collaborator Author

BCerki commented Jul 7, 2023

looks great star_struck I'm just not sure about creating snapshots from form pages; we need to change them every time we change a single line in those form pages. what do you think?!

I'm not attached to the snapshot tests. I added them based on your comment in the old PR, It would be nice to test the text of the tooltips in unit tests and make sure we won't change accidentally later. (jest assertions seemed like overkill for accidental text changes). Thoughts? Now that the tooltip text is separate from the ui schemas, I think it would be hard to accidentally change the text.

@Sepehr-Sobhani
Copy link
Contributor

looks great star_struck I'm just not sure about creating snapshots from form pages; we need to change them every time we change a single line in those form pages. what do you think?!

I'm not attached to the snapshot tests. I added them based on your comment in the old PR, It would be nice to test the text of the tooltips in unit tests and make sure we won't change accidentally later. (jest assertions seemed like overkill for accidental text changes). Thoughts? Now that the tooltip text is separate from the ui schemas, I think it would be hard to accidentally change the text.

My main concern was inadvertently changing certain texts, but a single unit test would ensure that they are not altered by mistake. However, the new approach of keeping them in a separate file minimizes the risk of accidental changes. We are now good to proceed.

@sam-warren sam-warren force-pushed the 1692-tooltips branch 3 times, most recently from d7301a7 to 3429aca Compare July 13, 2023 23:24
Copy link
Contributor

@sam-warren sam-warren left a comment

Choose a reason for hiding this comment

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

Changes look great 😃

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