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

Rules page #60

Open
wants to merge 21 commits into
base: dev
Choose a base branch
from
Open

Rules page #60

wants to merge 21 commits into from

Conversation

MoaKK
Copy link

@MoaKK MoaKK commented Oct 17, 2024

First attempt at creating rules page, open for reviews and suggestions.

Created rules page, necessary mock-data and components, and routing to subpages,

Have only implemented a temporary 'design' of how I want the page to look. Don't think i used the proper approach to using components and/or creating components. Would therefore like suggestions on how i could improve or refractor the actual code, current design of the page, improper use of tailwind properties, or any other areas of improvements.

Closes #18.

.env.example Outdated Show resolved Hide resolved
src/app/[locale]/(default)/rules/[subset]/page.tsx Outdated Show resolved Hide resolved
src/app/[locale]/(default)/rules/[subset]/page.tsx Outdated Show resolved Hide resolved
src/app/[locale]/(default)/rules/layout.tsx Outdated Show resolved Hide resolved
src/components/rules/RulesPagesList.tsx Outdated Show resolved Hide resolved
src/app/[locale]/(default)/rules/page.tsx Outdated Show resolved Hide resolved
src/lib/locale/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@michaelbrusegard michaelbrusegard left a comment

Choose a reason for hiding this comment

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

I just looked at the code right now, but I also think the styling can be improved. Maybe look at some other websites or examples and try to get inspiration. Also you can look at more "fancy" components to add to learn about that and it can be fun to work on, like aceturnity etc

src/app/[locale]/(default)/rules/[subset]/page.tsx Outdated Show resolved Hide resolved
src/app/[locale]/(default)/rules/layout.tsx Outdated Show resolved Hide resolved
src/app/[locale]/(default)/rules/page.tsx Outdated Show resolved Hide resolved
src/app/[locale]/(default)/rules/page.tsx Outdated Show resolved Hide resolved
src/components/rules/RulesPagesList.tsx Outdated Show resolved Hide resolved
src/components/rules/SubPages.tsx Outdated Show resolved Hide resolved
src/components/rules/SubPages.tsx Outdated Show resolved Hide resolved
src/components/rules/RulesPagesList.tsx Outdated Show resolved Hide resolved
src/components/rules/SubPages.tsx Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
Copy link
Member

@ZeroWave022 ZeroWave022 left a comment

Choose a reason for hiding this comment

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

Please also merge dev into your branch, run bun install, and take a look at other pages for examples of how to migrate to Next.js 15. Most notably, pages need to be async and we await params now, so a little different (but this can be copy-pasted from other pages)

messages/no.json Outdated Show resolved Hide resolved
src/app/[locale]/(default)/rules/(main)/loading.tsx Outdated Show resolved Hide resolved
src/app/[locale]/(default)/rules/(main)/page.tsx Outdated Show resolved Hide resolved
src/app/[locale]/(default)/rules/[subset]/page.tsx Outdated Show resolved Hide resolved
src/components/rules/RuleCardList.tsx Outdated Show resolved Hide resolved
src/components/rules/RuleCardList.tsx Outdated Show resolved Hide resolved
src/components/rules/RuleCardList.tsx Outdated Show resolved Hide resolved
@MoaKK MoaKK added the enhancement New feature or request label Jan 16, 2025
@ZeroWave022 ZeroWave022 self-requested a review January 17, 2025 17:30
.env.example Outdated Show resolved Hide resolved
src/components/rules/RuleCard.tsx Outdated Show resolved Hide resolved
}}
aria-label={title}
>
<Card className='flex size-full transform overflow-hidden rounded-xl brightness-95 transition delay-150 duration-300 ease-in-out hover:scale-105 hover:border-primary hover:shadow-lg hover:brightness-100 dark:brightness-100 hover:dark:brightness-110'>
Copy link
Member

Choose a reason for hiding this comment

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

I think dark:brightness-100 wont do anything here as it is the default. What is the reason we apply a brightness filter and lower the brightness by default?

src/components/rules/RuleCard.tsx Outdated Show resolved Hide resolved
src/components/rules/RuleCardList.tsx Outdated Show resolved Hide resolved
function RuleCardSkeleton() {
return (
<Button
className={cx('group whitespace-normal font-normal ring-0')}
Copy link
Member

Choose a reason for hiding this comment

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

Take note of the changes i wrote about on the RuleCard and apply them here too

Copy link
Member

Choose a reason for hiding this comment

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

Also you shouldnt use a button for a skeleton it is an interactive element.

variant='none'
size='none'
>
<Card className='mb-3 h-[68px] w-full max-w-[614px] shrink transform overflow-hidden rounded-xl brightness-95 transition delay-150 duration-300 ease-in-out hover:scale-105 hover:border-primary hover:shadow-lg hover:brightness-100 md:h-[138px] dark:brightness-100 hover:dark:brightness-110'>
Copy link
Member

Choose a reason for hiding this comment

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

Remove interactive styles. a skeleton should not be interactive and have hover effects etc

@michaelbrusegard
Copy link
Member

As for the design, I really like it. But the hover effect should probably be made faster. Also I am not sure about why we need to write internal rules on every single internal rules page when they are all in the same list and some other inconsistancies , but it is probably good enough for now.

@michaelbrusegard
Copy link
Member

Also I was a little strict, but it is probably fine so if you disagree or I misunderstood you can probably just resolve the comment!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Rules page
3 participants