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

Add hover animation to eventList #4998

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

magnusbrecke
Copy link
Contributor

Description

Added a hover effect over the event items in eventlist

Result

Before:

Skjermopptak.2024-09-25.kl.21.48.07.mov

After:

Skjermopptak.2024-09-25.kl.21.51.06.mov
  • [ x ] Changes look good on both light and dark theme.
  • [ x ] Changes look good with different viewports (mobile, tablet, etc.).
  • [ x ] Changes look good with slower Internet connections.

Testing

  • [ x ] I have thoroughly tested my changes.

Please describe what and how the changes have been tested, and provide instructions to reproduce if necessary.


Resolves ABA-1084

Copy link

linear bot commented Sep 25, 2024

@github-actions github-actions bot added the review-needed Pull requests that need review label Sep 25, 2024
Copy link
Contributor

@falbru falbru left a comment

Choose a reason for hiding this comment

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

Very nice work! Looks great 🔥

I forgot to mention it, but you don't actually need to use TypeScript in order to track if an element is hovered or not. You can use the CSS selector :hover instead!

app/components/EventItem/styles.css Outdated Show resolved Hide resolved
app/components/EventItem/styles.css Outdated Show resolved Hide resolved
@sudhanshuku01

This comment was marked as off-topic.

Copy link
Member

@ivarnakken ivarnakken left a comment

Choose a reason for hiding this comment

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

I have wanted this to be a link for so long!😍 Very nice!!

app/components/EventItem/styles.css Outdated Show resolved Hide resolved
app/components/EventItem/styles.css Outdated Show resolved Hide resolved
app/components/EventItem/styles.css Outdated Show resolved Hide resolved
@ivarnakken ivarnakken added enhancement Pull requests that make enhancements, instead of just purely new features changes-requested Pull requests with requested changes labels Sep 26, 2024
Copy link

vercel bot commented Sep 27, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
lego-bricks-storybook ⬜️ Ignored (Inspect) Visit Preview Oct 2, 2024 6:39pm

@magnusbrecke
Copy link
Contributor Author

New Changes:

Skjermopptak.2024-09-27.kl.12.17.15.mov

Copy link
Contributor

@falbru falbru left a comment

Choose a reason for hiding this comment

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

Awesome!

Copy link
Member

@eikhr eikhr left a comment

Choose a reason for hiding this comment

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

Looks very good!

Copy link
Member

@ivarnakken ivarnakken left a comment

Choose a reason for hiding this comment

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

Looking better!

Comment on lines 62 to 64
.eventItem:hover .companyLogo img {
border-radius: var(--border-radius-sm);
}
Copy link
Member

@ivarnakken ivarnakken Sep 27, 2024

Choose a reason for hiding this comment

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

This feels off - why are we adding border-radius to the images as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried both with and without, but personally felt with felt more natural. No other reason than that

Copy link
Member

Choose a reason for hiding this comment

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

You don't have to fix this now in this PR, but it would be nice if you did the same to the other event item types 😍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree I have already started working on it!

@magnusbrecke
Copy link
Contributor Author

New additions to the page where the hover effect has been implemented:

Profile page:

Skjermopptak.2024-10-02.kl.18.53.46.mov

Authenticated front-page:

Skjermopptak.2024-10-02.kl.18.55.40.mov

Copy link
Contributor

@itsisak itsisak left a comment

Choose a reason for hiding this comment

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

LGTM.

In the future it could be nice with a standardized component for a link with this effect. But this is good for now!

app/components/EventItem/index.tsx Outdated Show resolved Hide resolved
Copy link
Member

@eikhr eikhr left a comment

Choose a reason for hiding this comment

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

Huge improvement!

Comment on lines 11 to 16
}

.eventItem:hover {
background-color: var(--additive-background);
border-radius: var(--border-radius-sm);
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}
.eventItem:hover {
background-color: var(--additive-background);
border-radius: var(--border-radius-sm);
}
&:hover {
background-color: var(--additive-background);
border-radius: var(--border-radius-sm);
}
}

Use &:hover nested inside the class to add hover styling to the same classname:)


.eventItem:hover {
background-color: var(--additive-background);
border-radius: var(--border-radius-sm);
Copy link
Member

Choose a reason for hiding this comment

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

Joblistings use --border-radius-lg. I would love for this to be consistant with that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also removed the radius from the left side
It is necessary when the bordrer radius is that big

@eikhr
Copy link
Member

eikhr commented Oct 2, 2024

We should add this to articles and meetings too, preferably with a shared component to ensure consistency. But I think this is good now, and that can be done in a separate PR:)

Copy link
Contributor

@falbru falbru left a comment

Choose a reason for hiding this comment

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

Awesome

@ivarnakken
Copy link
Member

Remember to squash merge all the commits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes-requested Pull requests with requested changes enhancement Pull requests that make enhancements, instead of just purely new features review-needed Pull requests that need review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants