Skip to content

fix(theme): use correct tokens for the breadcrumbs #88

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 24, 2024
Merged

fix(theme): use correct tokens for the breadcrumbs #88

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
597e4cb
fix(theme): use correct tokens for the breadcrumbs
SamVerschueren Jun 21, 2024
e222956
chore: fix prettier
SamVerschueren Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion packages/astro/src/default/styles/variables.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,13 +273,20 @@
--tk-elements-breadcrumbs-dropdown-textColor: var(--tk-elements-app-textColor);
--tk-elements-breadcrumbs-dropdown-textColorHover: var(--tk-text-accent);
--tk-elements-breadcrumbs-dropdown-accordionTextColor: var(--tk-elements-breadcrumbs-dropdown-textColor);
--tk-elements-breadcrumbs-dropdown-accordionTextColorSelected: var(
--tk-elements-breadcrumbs-dropdown-accordionTextColor
);
--tk-elements-breadcrumbs-dropdown-accordionTextColorHover: var(--tk-text-active);
--tk-elements-breadcrumbs-dropdown-accordionIconColor: var(--tk-text-disabled);
--tk-elements-breadcrumbs-dropdown-accordionIconColorSelected: var(
--tk-elements-breadcrumbs-dropdown-accordionIconColor
);
--tk-elements-breadcrumbs-dropdown-accordionIconColorHover: var(--tk-text-primary);
--tk-elements-breadcrumbs-dropdown-lessonBackgroundColor: var(--tk-elements-breadcrumbs-dropdown-backgroundColor);
--tk-elements-breadcrumbs-dropdown-lessonBackgroundColorSelected: var(--tk-background-secondary);
--tk-elements-breadcrumbs-dropdown-lessonTextColor: var(--tk-elements-breadcrumbs-dropdown-textColor);
--tk-elements-breadcrumbs-dropdown-lessonTextColorHover: var(--tk-elements-breadcrumbs-dropdon-textColorHover);
--tk-elements-breadcrumbs-dropdown-lessonTextColorSelected: var(--tk-elements-breadcrumbs-dropdown-lessonTextColor);
--tk-elements-breadcrumbs-dropdown-lessonTextColorHover: var(--tk-elements-breadcrumbs-dropdown-textColorHover);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This had a typo dropdon instead of dropdown.


/* Terminal */
--tk-elements-terminal-backgroundColor: var(--tk-background-active-secondary);
Expand Down
3 changes: 3 additions & 0 deletions packages/astro/src/theme.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,12 +275,15 @@ export const theme: ConfigBase['theme'] = {
textColor: 'var(--tk-elements-breadcrumbs-dropdown-textColor)',
textColorHover: 'var(--tk-elements-breadcrumbs-dropdown-textColorHover)',
accordionTextColor: 'var(--tk-elements-breadcrumbs-dropdown-accordionTextColor)',
accordionTextColorSelected: 'var(--tk-elements-breadcrumbs-dropdown-accordionTextColorSelected)',
accordionTextColorHover: 'var(--tk-elements-breadcrumbs-dropdown-accordionTextColorHover)',
accordionIconColor: 'var(--tk-elements-breadcrumbs-dropdown-accordionIconColor)',
accordionIconColorSelected: 'var(--tk-elements-breadcrumbs-dropdown-accordionIconColorSelected)',
accordionIconColorHover: 'var(--tk-elements-breadcrumbs-dropdown-accordionIconColorHover)',
lessonBackgroundColor: 'var(--tk-elements-breadcrumbs-dropdown-lessonBackgroundColor)',
lessonBackgroundColorSelected: 'var(--tk-elements-breadcrumbs-dropdown-lessonBackgroundColorSelected)',
lessonTextColor: 'var(--tk-elements-breadcrumbs-dropdown-lessonTextColor)',
lessonTextColorSelected: 'var(--tk-elements-breadcrumbs-dropdown-lessonTextColorSelected)',
lessonTextColorHover: 'var(--tk-elements-breadcrumbs-dropdown-lessonTextColorHover)',
},
},
Expand Down
27 changes: 15 additions & 12 deletions packages/components/react/src/Nav.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,14 +109,12 @@ function renderParts(navList: NavList, currentLesson: Lesson) {
navStyles.AccordionTrigger,
'flex items-center gap-1 w-full hover:text-primary-700',
{
'font-semibold': isPartActive,
[`font-semibold ${navStyles.AccordionTriggerActive}`]: isPartActive,
},
)}
>
<span
className={`${navStyles.AccordionTriggerIcon} i-ph-caret-right-bold scale-80 text-tk-elements-breadcrumbs-dropdown-accordionToggleIconColor`}
></span>
<span className={navStyles.AccordionTriggerText}>{`Part ${partIndex + 1}: ${part.title}`}</span>
<span className={`${navStyles.AccordionTriggerIcon} i-ph-caret-right-bold scale-80`}></span>
<span>{`Part ${partIndex + 1}: ${part.title}`}</span>
Copy link
Member

Choose a reason for hiding this comment

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

Oh I hadn't realised that we injected Part <index> in the title. This is not ideal when writing a Tutorial in a language other than English.

I feel like we should remove it but not in this PR though.

Curious to hear your thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OH you're right! We should do this differently.

</Accordion.Trigger>
<Accordion.Content className={navStyles.AccordionContent}>
{renderChapters(currentLesson, part, isPartActive)}
Expand Down Expand Up @@ -150,16 +148,20 @@ function renderChapters(currentLesson: Lesson, part: NavItem, isPartActive: bool
navStyles.AccordionTrigger,
'flex items-center gap-1 w-full hover:text-primary-700',
{
'font-semibold': isChapterActive,
[`font-semibold ${navStyles.AccordionTriggerActive}`]: isChapterActive,
},
)}
>
<span
className={`${navStyles.AccordionTriggerIcon} i-ph-caret-right-bold scale-80 text-gray-300`}
className={classNames(
navStyles.AccordionTriggerIcon,
'i-ph-caret-right-bold scale-80 text-gray-300',
{
[navStyles.AccordionTriggerActive]: isChapterActive,
},
)}
></span>
<span className="text-tk-elements-breadcrumbs-dropdown-accordionTextColor hover:text-tk-elements-breadcrumbs-dropdown-accordionTextColorHover">
{chapter.title}
</span>
<span>{chapter.title}</span>
</Accordion.Trigger>
<Accordion.Content className={navStyles.AccordionContent}>
{renderLessons(currentLesson, chapter, isPartActive, isChapterActive)}
Expand All @@ -183,10 +185,11 @@ function renderLessons(currentLesson: Lesson, chapter: NavItem, isPartActive: bo
<li key={lessonIndex} className="mr-3">
<a
className={classNames(
'w-full inline-block border border-transparent pr-3 hover:text-tk-elements-breadcrumbs-dropdown-textColorHover px-3 py-1 rounded-1',
'w-full inline-block border border-transparent pr-3 text-tk-elements-breadcrumbs-dropdown-lessonTextColor hover:text-tk-elements-breadcrumbs-dropdown-lessonTextColorHover px-3 py-1 rounded-1',
{
'bg-tk-elements-breadcrumbs-dropdown-lessonBackgroundColor': !isActiveLesson,
'font-semibold bg-tk-elements-breadcrumbs-dropdown-lessonBackgroundColorSelected': isActiveLesson,
'font-semibold text-tk-elements-breadcrumbs-dropdown-lessonTextColorSelected bg-tk-elements-breadcrumbs-dropdown-lessonBackgroundColorSelected':
isActiveLesson,
},
)}
href={lesson.href}
Expand Down
8 changes: 8 additions & 0 deletions packages/components/react/src/styles/nav.module.css
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

Do you know why we have those classes? It's the first time I'm noticing this CSS module. 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first I was moving all the UNO-css things inline. But then I realized it was annoying because that meant I had to copy and paste that every time the accordion trigger component is used.

So the classes are there (I think), so that you can just set navStyles.AccordionTrigger on the trigger, and the correct CSS is applied and you don't have to repeat the uno-css styles.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see. I would argue we should have "presentational" components though to avoid the repetition.

We can always revisit that later 👍

Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@
--at-apply: text-tk-elements-breadcrumbs-dropdown-accordionTextColor;
}

.AccordionTrigger.AccordionTriggerActive {
--at-apply: text-tk-elements-breadcrumbs-dropdown-accordionTextColorSelected;
}

.AccordionTrigger:hover {
--at-apply: text-tk-elements-breadcrumbs-dropdown-accordionTextColorHover;
}
Expand All @@ -14,6 +18,10 @@
--at-apply: transition-color duration-100 text-tk-elements-breadcrumbs-dropdown-accordionIconColor;
}

.AccordionTrigger.AccordionTriggerActive .AccordionTriggerIcon {
--at-apply: text-tk-elements-breadcrumbs-dropdown-accordionIconColorSelected;
}

.AccordionTrigger:hover .AccordionTriggerIcon {
--at-apply: text-tk-elements-breadcrumbs-dropdown-accordionIconColorHover;
}
Expand Down