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

remove deprecated typography components #1296

Merged
merged 8 commits into from May 23, 2023
Merged

Conversation

mayank99
Copy link
Contributor

@mayank99 mayank99 commented May 18, 2023

Changes

yeet!

I'm working on some repo-wide changes and these were getting in the way.

Testing

N/A

Docs

https://github.com/iTwin/iTwinUI/wiki/iTwinUI-react-v3-migration-guide#typography

@mayank99 mayank99 added this to the React 3.0 milestone May 18, 2023
@mayank99 mayank99 self-assigned this May 18, 2023
@mayank99 mayank99 marked this pull request as ready for review May 18, 2023 18:13
@mayank99 mayank99 requested a review from a team as a code owner May 18, 2023 18:13
@mayank99 mayank99 requested review from a team, gretanausedaite and adamhock and removed request for a team May 18, 2023 18:13
@gretanausedaite
Copy link
Contributor

I'm working on some repo-wide changes and these were getting in the way.

What was the problem?

@mayank99
Copy link
Contributor Author

I'm working on some repo-wide changes and these were getting in the way.

What was the problem?

It was for #1298. These typography components don't use Text internally, so I would have needed to make changes in every single one of them individually. Decided to remove them instead, as there's no point.

Copy link
Contributor

@gretanausedaite gretanausedaite left a comment

Choose a reason for hiding this comment

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

Do we have any recommendation for users how to use Text component in their apps? How to add correct padding?
eg. wrap in div or span or something.

@mayank99
Copy link
Contributor Author

Oh, do you mean the iui-text-spacing class? iirc we were not happy with the spacing it adds because it is undesirable in most cases and even when it's desirable, it can be too large (constant 22px). But I don't think we have any recommendations

/cc @FlyersPh9

@gretanausedaite
Copy link
Contributor

Oh, do you mean the iui-text-spacing class? iirc we were not happy with the spacing it adds because it is undesirable in most cases and even when it's desirable, it can be too large (constant 22px). But I don't think we have any recommendations

Yeah, when user wants to add padding at the bottom of <Text variant='heading'/>, what is the recommended way to do that. We definitely should document which spacing variables to use with which variant and show some examples how to do that in our docs site.

@mayank99
Copy link
Contributor Author

It's a bit difficult to even come up with a recommendation. We can't just say something like "add margin-bottom: var(--iui-size-m)". The desired value will be different in every case.

Maybe we can just say "use margin or gap with one of the values from itwinui-variables".

@gretanausedaite
Copy link
Contributor

Maybe we can just say "use margin or gap with one of the values from itwinui-variables".

Lets move the discussion out of PR. It's documentation issue, not this PR.

@mayank99 mayank99 added this pull request to the merge queue May 23, 2023
@mayank99 mayank99 mentioned this pull request May 23, 2023
22 tasks
@mayank99 mayank99 removed this pull request from the merge queue due to a manual request May 23, 2023
@mayank99 mayank99 merged commit e5c8b4f into dev May 23, 2023
9 checks passed
@mayank99 mayank99 deleted the mayank/deprecated-typography branch May 23, 2023 15:19
@imodeljs-admin imodeljs-admin mentioned this pull request Oct 23, 2023
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.

None yet

2 participants