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

refactor: remove tailwind utilities in src/view files #236

Conversation

andrewwylde
Copy link
Contributor

No description provided.

@andrewwylde andrewwylde requested a review from a team as a code owner September 8, 2023 15:56
Copy link
Contributor

Choose a reason for hiding this comment

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

I updated the three components in my PR, so maybe we can remove these from this PR to reduce conflicts.

Copy link
Contributor

@davidma415 davidma415 left a comment

Choose a reason for hiding this comment

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

Left a few comments

@andrewwylde andrewwylde changed the title feat(lint): remove tailwind utilities in src/view files refactor: remove tailwind utilities in src/view files Sep 12, 2023
@andrewwylde andrewwylde force-pushed the fix/remove-tailwind-utilities-view-folder branch from 91acd4e to 8958e11 Compare September 12, 2023 15:57
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidma415 I think this still needs float and cursor adjustments from beforetimes

Copy link
Contributor

Choose a reason for hiding this comment

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

the .popover class on KPop has float applied. But yeah, looks like I missed cursor pointer on actions-badge class!

/>
</div>
<div class="flex">
<div class="buttons-wrapper flex">
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to remove flex

/>
</div>
<div class="flex">
<div class="buttons-wrapper flex">
<div class="flex-1">
Copy link
Contributor

Choose a reason for hiding this comment

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

also remove flex-1

Comment on lines 506 to 507
font-size: 0.875rem;
line-height: 1.25rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we change this to use the design tokens, or the closest values at least? should probably move away from using rem.

Comment on lines 70 to 72
display: flex;
align-items: center;
justify-content: center;
Copy link
Contributor

Choose a reason for hiding this comment

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

with h1, .circle these flex styles would be applied to the h1 as well - is this intended? If not, we would want to target .circle separately.

Comment on lines 71 to 73
display: flex;
align-items: center;
justify-content: center;
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment, i don't think we want to target h1 with the flex styles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh thanks

@@ -1,9 +1,9 @@
<template>
<div class="container flex pb-0 product fixed-position">
<div class="product-shell product fixed-position responsive-container">
Copy link
Contributor

Choose a reason for hiding this comment

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

we should remove fixed-position as well.

@andrewwylde andrewwylde merged commit 2a8d731 into feat/lint-rules-remove-kongponents-utils Sep 14, 2023
2 of 3 checks passed
@andrewwylde andrewwylde deleted the fix/remove-tailwind-utilities-view-folder branch September 14, 2023 17:50
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.

2 participants