Skip to content

Conversation

@crnkovic
Copy link
Contributor

@crnkovic crnkovic commented Aug 30, 2022

Summary

https://app.clickup.com/t/2u58u58

Checklist

  • I checked my UI changes against the design and there are no notable differences
  • I checked my UI changes for any responsiveness issues
  • I checked my (code) changes for obvious issues, debug statements and commented code
  • I provided a screenshot of my changes to the component (if applicable)
  • I regenerated the icons.html file and checked if my newly added icon is shown correctly (if necessary)
  • I added an explanation on how to use the component to the readme (if necessary)
  • Documentation (if necessary)
  • Tests (if necessary)
  • Ready to be merged

@crnkovic crnkovic marked this pull request as ready for review September 16, 2022 07:52
Comment on lines 12 to 14
.rich-select-dropdown-entry:not(.rich-select-dropdown-entry-selected):not(.rich-select-dropdown-entry-hover) {
.rich-select-dropdown-entry:not(.rich-select-dropdown-entry-selected):not(
.rich-select-dropdown-entry-hover
) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

formatting

.navbar-container {
@apply px-10;
}
@apply px-8 md:px-10;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

replaced the @screen with md:px-10

Comment on lines -26 to +20
@apply font-bold text-theme-secondary-900;
}

h1 {
@apply font-bold;
}

.dark h1,
.dark h2,
.dark h3,
.dark h4 {
@apply text-theme-secondary-200;
@apply font-bold text-theme-secondary-900 dark:text-theme-secondary-200 leading-tight;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

additional font-bold on h1 was unnecessary. also .dark h1 can be replaced with dark:text-theme...

Comment on lines -45 to +28
@apply font-bold text-theme-secondary-900;
}

.header-1 {
@apply font-bold text-theme-secondary-900;
}

.dark header-1,
.dark header-2,
.dark header-3,
.dark header-4 {
@apply text-theme-secondary-200;
@apply font-bold text-theme-secondary-900 dark:text-theme-secondary-200 leading-tight;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as with .dark h1...

h1,
.header-1 {
@apply text-3xl mb-1;
@apply text-3xl mb-1 md:text-4xl md:mb-2;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

replaced @screen with this

h1,
.header-1 {
@apply text-4xl mb-2;
@layer base {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pushed core styles to base layer

.rotate-180 {
transform: rotate(180deg);
}
@layer utilities {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pushed clear-transform and rotate-minus-90 to utilities layer. though they may not be even used in any of the projects, i wouldn't remove them here now

@ItsANameToo ItsANameToo changed the title refactor: push various CSS class definitions to Tailwind layers and consolidate heading classes refactor: consolidate heading classes and use tailwind layers Jan 13, 2023
@ItsANameToo ItsANameToo merged commit 7137b84 into main Jan 13, 2023
@ItsANameToo ItsANameToo deleted the refactor/heading-classes branch January 13, 2023 14:10
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