Use design palette highlight color (#f8eedf) for all page headers#168
Use design palette highlight color (#f8eedf) for all page headers#168Copilot wants to merge 6 commits into
Conversation
|
@aaditi2 can you check the colors used on all the pages throughout the website to make sure that on more than OS and on {Chrome, Firefox, Edge, Brave} browsers that the text adheres to the colors specified in design color palettes and shows contrast with the background. This is documented in issue #159 |
Agent-Logs-Url: https://github.com/codewit-us/codewit.us/sessions/e7322396-01ee-458e-ad35-94cfcd42a3b4 Co-authored-by: kbuffardi <8324410+kbuffardi@users.noreply.github.com>
|
@kbuffardi yes I'll check it |
|
@kbuffardi Yes it seems like the issue is still persistent. I can still see gray color headings instead of #f8eedf for the headings on Chrome, Firefox and Edge. I checked Chrome through my mac and other 2 through Windows and it gave the same issue. |
Thanks. Can you go through all the unique pages and create a list of the pages that had any gray headings/text? |
|
Hi @kbuffardi, I’ve verified the theme changes. Most are looking great, though I've found a slight discrepancy in the text color for the exercise view. Correct intended views:
Issue:
|
modified the changes to use a new set of components in the `typeography.tsx` file. this will hold common styling elements related to text components. I left some notes in the file regarding the current state of it.
vite did not feel like telling me about it and had to force it to refresh.
NickK21
left a comment
NickK21
left a comment
There was a problem hiding this comment.
@DAC098 requesting changes on the shared heading helper, since the current implementation introduces jsx-a11y/heading-has-content warnings by relying on {...props} for children. Rendering children explicitly in the wrapper should preserve the behavior and avoid the added lint noise.
The "typeography" to "typography" filename spelling and the TeacherView heading change are smaller follow-up items.
| export const H1 = forwardRef<HTMLHeadingElement, ComponentProps<"h1">>(({className, ...props}, ref) => { | ||
| return <h1 | ||
| ref={ref} | ||
| className={cn("text-highlight-500", className)} | ||
| {...props} | ||
| />; | ||
| }); | ||
| H1.displayName = "H1"; |
There was a problem hiding this comment.
Nice abstraction overall.
One thing to maybe tweak: these heading wrappers pass children through {...props}, which still renders correctly, but eslint can’t detect the heading content and raises jsx-a11y/heading-has-content on each helper.
Could we destructure children explicitly and render it inside the heading element? That should keep the behavior the same and avoid the new lint noise.
There was a problem hiding this comment.
this was a similar process as to what I have done with a different project. the idea being that they only thing that needs to be extracted is and then everything else is just passed through without worrying about what it is.
if we are starting to use eslint then I can make the change otherwise I am inclined to leave it as me taking it out just to manually pass it in seems unnecessary.
There was a problem hiding this comment.
Non-blocking: should this file be renamed to typography.tsx before more imports depend on the typeography spelling?
There was a problem hiding this comment.
yes, I should probably fix my bad spelling
| <H1 className="text-[16px] font-bold"> | ||
| Teacher Dashboard | ||
| </span> | ||
| </H1> |
There was a problem hiding this comment.
I noticed this changes Teacher Dashboard from a span to an h1 via the new helper.
I assume that was intentional, but wanted to confirm the goal here was also to make it a semantic page heading, not just to pick up the shared heading styling.
There was a problem hiding this comment.
that was the idea, since it had the exact same heading style I figured I would just make it a heading.
Headers across multiple pages were rendering in faint grey — either from undefined utility classes (
text-heading,text-body) falling back to Toastify's--toastify-text-color-light: #757575, fromtext-foreground-200(#b3adaa), or from having no color class at all.Changes
Home.tsx—h2"Instructing" / "Attending": no color →text-highlight-500exercise_id.tsx—h2/h3/h4using undefinedtext-heading→text-highlight-500;<p>using undefinedtext-body→text-whiteImportExercisesPanel.tsx—h3text-foreground-200→text-highlight-500TeacherView.tsx—h1/h2text-foreground-200→text-highlight-500Read.tsx—h3"No Exercises": no color →text-highlight-500highlight-500is already defined intailwind.config.jsas#f8eedf: