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

Enhance the Card component #3488

Merged
merged 2 commits into from Jan 31, 2023
Merged

Enhance the Card component #3488

merged 2 commits into from Jan 31, 2023

Conversation

ivarnakken
Copy link
Member

@ivarnakken ivarnakken commented Jan 29, 2023

Description

Enhance the Card component

Also make all "cards" actually employ the component. The cards now have
a much more modern look and they all share the same styling.


Iterate on the introduction of Inter

Small styling changes here and there. Among other things, the background is now less "blue".

Result

Before
image

After
image

Before
image

After
The cards looks much better here once the content background is gone.

image

Testing

  • I have thoroughly tested my changes.

I've tested all use cases of the component.


Resolves ABA-225

@ivarnakken ivarnakken added enhancement Pull requests that make enhancements, instead of just purely new features future-is-now Pull requests that utilize cutting-edge technology or just a cool new feature review-needed Pull requests that need review labels Jan 29, 2023
@ivarnakken ivarnakken requested a review from a team January 29, 2023 17:04
@linear
Copy link

linear bot commented Jan 29, 2023

ABA-225 Enhance Card component

Improve the design and make other "cards" actually use the component in order to standardize the look of the webapp.


html[data-theme='dark'] .card {
background: inherit;
border-radius: 1rem;
Copy link
Member Author

Choose a reason for hiding this comment

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

This matches the border-radius of the newly improved modals.

Comment on lines +13 to +19
let format =
moment().year() === moment(item.startTime).year()
? 'DD. MMM'
: 'DD. MMM YYYY';
if (isEvent) {
format += ' HH:mm';
}
Copy link
Member Author

Choose a reason for hiding this comment

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

The exact time the article was posted isn't that important. However, it's different for events because it's important to know when it starts.

Copy link
Member

@LudvigHz LudvigHz left a comment

Choose a reason for hiding this comment

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

Awesome. This is a much much needed improvement 🚀

app/components/Card/Card.css Show resolved Hide resolved
@LudvigHz
Copy link
Member

LudvigHz commented Jan 29, 2023

IMO the borders were increased a tad too much. I'd probably enjoy something in-between this and the current more.
Also, the contrast between the card and the background is a bit too small for my liking (on the frontpage as well). I think ether some more pronounced border or more difference in the card/bg color would make this a bit better

@ivarnakken
Copy link
Member Author

IMO the borders were increased a tad too much. I'd probably enjoy something in-between this and the current more.

I don't really agree. A 0.2rem diff isn't going to do that much. I think this looks much more modern.

Also, the contrast between the card and the background is a bit too small for my liking (on the frontpage as well). I think ether some more pronounced border or more difference in the card/bg color would make this a bit better

Well, yes, but no. You see, that's kind of the point. I tried a higher contrast, but that just looked off. It made the cards stand out way too much and they became too eye-catching, which is not its purpose. You want it to blend in nicely with the surroundings, while still separating its content from the rest.

@@ -130,7 +130,7 @@ const Footer = ({ loggedIn }: Props) => (
Informasjonskapsler (cookies)
</Link>
<Link to="/pages/personvern/124-personvernserklring">
Personvernserklæring
Personvernerklæring
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to be done. Sorry. I'm not making a seperate PR for this.

Copy link
Contributor

@ollfkaih ollfkaih left a comment

Choose a reason for hiding this comment

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

Great work!

app/components/RandomQuote/RandomQuote.css Show resolved Hide resolved
app/components/Header/toggleTheme.css Show resolved Hide resolved
Also make all "cards" actually employ the component. The cards now have
a much more modern look and they all share the same styling.
Small styling changes here and there. Among other things, the background is now less "blue".
@ivarnakken
Copy link
Member Author

The contrast between the card and the background has been increased.

@ivarnakken ivarnakken added the approved Pull requests that have been approved label Jan 30, 2023
@ivarnakken ivarnakken merged commit c404066 into master Jan 31, 2023
@ivarnakken ivarnakken deleted the card branch January 31, 2023 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Pull requests that have been approved enhancement Pull requests that make enhancements, instead of just purely new features future-is-now Pull requests that utilize cutting-edge technology or just a cool new feature review-needed Pull requests that need review
Projects
None yet
4 participants