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.

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

Jump to bottom

appearance settings changes #189

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

appearance settings changes #189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 6 additions & 1 deletion app/chapter/[chapterNumber]/(chapter)/chapter-page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,12 @@ export default function ChapterPage({
: filteredVerses?.slice(0).reverse();

return (
<div>
<div
className={classNames(
"font-inter dark:bg-dark-bg",
`bg-${styles.backgroundColor}`
)}
>
Comment on lines +45 to +50
Copy link
Contributor

Choose a reason for hiding this comment

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

Image description CodeRabbit

The dynamic class name bg-${styles.backgroundColor} might not work as expected if the value of styles.backgroundColor is not a valid Tailwind CSS color. If styles.backgroundColor can be an arbitrary color, consider using inline styles instead.

-        `bg-${styles.backgroundColor}`
+        style={{ backgroundColor: styles.backgroundColor }}

<div className="absolute max-w-5xl font-inter left-0 right-0 mx-auto text-center">
<SvgChapterBackground className="relative text-gray-300 w-full lg:w-min dark:text-black text-opacity-25 dark:text-opacity-25 rounded-full m-auto left-0 right-0 bottom-0 lg:top-12" />
</div>
Expand Down
2 changes: 1 addition & 1 deletion components/Shared/Settings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ const Settings = ({ settingsIsOpen, closeSettingsModal }: Props) => {
className={classNames(
" text-gray-500 dark:text-gray-200",
appearenceSettings?.fontSize === "small"
? "text-md"
? "text-base"
: "text-xl",
appearenceSettings?.spacing === "large"
? "leading-loose"
Expand Down
6 changes: 4 additions & 2 deletions components/Verse/Commentary.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ export default function Commentary({ commentaryData }: Props) {
<h1
className={classNames(
"font-extrabold dark:text-gray-50",
styles.fontSize.heading
styles.fontSize.heading,
styles.lineHeight
Comment on lines +20 to +21
Copy link
Contributor

Choose a reason for hiding this comment

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

Image description CodeRabbit

The addition of styles.lineHeight to the className of the h1 tag is a good change as it allows for dynamic control over the line height. However, please ensure that styles.lineHeight always has a valid value and does not break the layout if undefined or null.

       <h1
         className={classNames(
           "font-extrabold dark:text-gray-50",
+          styles.fontSize.heading,
+          styles.lineHeight
         )}
       >
         Commentary

)}
>
Commentary
Expand All @@ -28,7 +29,8 @@ export default function Commentary({ commentaryData }: Props) {
key={paragraph}
className={classNames(
"mt-6 mx-auto text-justify dark:text-gray-50 whitespace-pre-wrap",
styles.fontSize.para
styles.fontSize.para,
styles.lineHeight
Comment on lines +32 to +33
Copy link
Contributor

Choose a reason for hiding this comment

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

Image description CodeRabbit

Similar to the previous comment, the addition of styles.lineHeight to the className of the paragraph tag is a good change as it allows for dynamic control over the line height. Again, please ensure that styles.lineHeight always has a valid value and does not break the layout if undefined or null.

             key={paragraph}
             className={classNames(
               "mt-6 mx-auto text-justify dark:text-gray-50 whitespace-pre-wrap",
+              styles.fontSize.para,
+              styles.lineHeight
             )}
           >
             {paragraph}

)}
>
{paragraph}
Expand Down
6 changes: 4 additions & 2 deletions components/Verse/Translation.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ export default function Translation({ translationData }: Props) {
<h1
className={classNames(
"font-extrabold dark:text-gray-50",
styles.fontSize.heading
styles.fontSize.heading,
styles.lineHeight
Comment on lines +17 to +18
Copy link
Contributor

Choose a reason for hiding this comment

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

Image description CodeRabbit

The addition of styles.lineHeight to the className is a good move for better control over the appearance. However, it's important to ensure that styles.lineHeight is always defined and has a valid CSS class value to prevent potential styling issues.

       <h1
         className={classNames(
           "font-extrabold dark:text-gray-50",
+          styles.fontSize.heading,
+          styles.lineHeight
         )}
       >
         Translation

)}
>
Translation
Expand All @@ -23,7 +24,8 @@ export default function Translation({ translationData }: Props) {
<p
className={classNames(
"mt-6 mx-auto text-justify dark:text-gray-50 ",
styles.fontSize.para
styles.fontSize.para,
styles.lineHeight
Comment on lines +27 to +28
Copy link
Contributor

Choose a reason for hiding this comment

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

Image description CodeRabbit

Similar to the previous comment, the addition of styles.lineHeight is a good move for better control over the appearance. Again, make sure that styles.lineHeight is always defined and has a valid CSS class value to prevent potential styling issues.

         <p
           className={classNames(
             "mt-6 mx-auto text-justify dark:text-gray-50 ",
+            styles.fontSize.para,
+            styles.lineHeight
           )}
         >
           {translationData[0].description}

)}
>
{translationData[0].description}
Expand Down
12 changes: 8 additions & 4 deletions components/Verse/Verse.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ export const Verse: FC<VerseProps> = ({
<h1
className={classNames(
"font-extrabold dark:text-gray-50",
styles.fontSize.heading
styles.fontSize.heading,
styles.lineHeight
)}
>
BG {chapter_number}.{verse_number}
Expand All @@ -59,7 +60,8 @@ export const Verse: FC<VerseProps> = ({
<p
className={classNames(
"font-dev text-my-orange mt-8 max-w-md mx-auto",
styles.fontSize.subHeading1
styles.fontSize.subHeading1,
styles.lineHeight
)}
>
{text}
Expand All @@ -75,7 +77,8 @@ export const Verse: FC<VerseProps> = ({
<p
className={classNames(
"mt-6 max-w-md mx-auto dark:text-gray-50",
styles.fontSize.subHeading2
styles.fontSize.subHeading2,
styles.lineHeight
)}
>
{transliteration}
Expand All @@ -86,7 +89,8 @@ export const Verse: FC<VerseProps> = ({
<p
className={classNames(
"mt-6 mx-auto dark:text-gray-50",
styles.fontSize.subHeading2
styles.fontSize.subHeading2,
styles.lineHeight
)}
>
{word_meanings}
Expand Down
6 changes: 3 additions & 3 deletions hooks/useMyStyles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ function useMyStyles() {
fontSize: {
heading: "text-4xl",
subHeading1: "text-intro",
subHeading2: "text-paragraph",
subHeading2: "text-xl",
Copy link
Contributor

Choose a reason for hiding this comment

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

Image description CodeRabbit

The change from "text-paragraph" to "text-xl" for subHeading2 might cause inconsistency in the appearance of subheadings across the application. Ensure that this change aligns with the overall design system and doesn't create visual discrepancies.

- subHeading2: "text-xl",
+ subHeading2: "text-paragraph",

para: "text-paragraph",
},
};
Expand All @@ -37,8 +37,8 @@ function useMyStyles() {
fontSize: {
heading: "text-3xl",
subHeading1: "text-2xl",
subHeading2: "text-xl",
para: "text-md",
subHeading2: "text-paragraph",
para: "text-base",
Comment on lines +40 to +41
Copy link
Contributor

Choose a reason for hiding this comment

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

Image description CodeRabbit

The changes in font sizes for subHeading2 and para could potentially impact readability. Make sure these changes are tested thoroughly for different screen sizes and devices to ensure a consistent user experience.

- subHeading2: "text-paragraph",
- para: "text-base",
+ subHeading2: "text-xl",
+ para: "text-md",

},
};
});
Expand Down
2 changes: 1 addition & 1 deletion redux/reducers/settings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ const initialState = {
loading: false,
fontSize: "large", //small, large todo: change to exact values
fontFamily: "proxima", //Tisa Pro, Georgia, Avenir, Proxima Nova
spacing: "large", //small, medium, large todo: change to exact values
spacing: "medium", //small, medium, large todo: change to exact values
bg: "bg-light-bg", //bg-light-bg, bg-yellow-bg, bg-dark-bg
currentVerse: initialVerse,
};
Expand Down
Loading