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

Make company descriptions collapsible #3594

Merged
merged 1 commit into from Feb 21, 2023
Merged

Conversation

PeterJFB
Copy link
Contributor

@PeterJFB PeterJFB commented Feb 19, 2023

Description

Make company descriptions over a certain length collapsible, making the succeeding components more accessible.

Result

Before
Screenshot from 2023-02-19 14-47-36

After
Long description:
Screencast from 02-19-2023 02:46:45 PM.webm

Forgot to record in light theme, but here's how that looks^
image

Short descriptions will not have the option to expand the content
Screenshot from 2023-02-19 14-48-17

Testing

  • I have thoroughly tested my changes.

Ensured that the component does not perform any unnecessary animation while loading into the page with short or long content. Made sure the interactions works nicely on mobile as well:)


Resolves ABA-294

@PeterJFB PeterJFB added the enhancement Pull requests that make enhancements, instead of just purely new features label Feb 19, 2023
@PeterJFB PeterJFB self-assigned this Feb 19, 2023
@github-actions github-actions bot added the review-needed Pull requests that need review label Feb 19, 2023
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.

Very cool! 💯💯

I have some minor comments, but they aren't that important. What bothers me more is the trigger icon on top of the text. Since it is outlined, it sort of blends in together with the text, and I'm not a big fan of it. With "show more / expanding content triggers" I prefer flat text buttons, like these:

image

But, whatever you end up with, please don't put it on top of the text. I like the "fade" a lot though!

@LudvigHz
Copy link
Member

Nice! 💯
But why not just add a collapse prop on displayContent instead of making a nearly identical one?

@ivarnakken ivarnakken added the changes-requested Pull requests with requested changes label Feb 19, 2023
@PeterJFB
Copy link
Contributor Author

Very cool! 100100

I have some minor comments, but they aren't that important. What bothers me more is the trigger icon on top of the text. Since it is outlined, it sort of blends in together with the text, and I'm not a big fan of it. With "show more / expanding content triggers" I prefer flat text buttons, like these:

image

But, whatever you end up with, please don't put it on top of the text. I like the "fade" a lot though!

Yeah i agree the outline was clashing with the text too much.

The updated look is on the light theme picture. I moved it and added a smooth background to it, think it looks way cleaner now^

@PeterJFB
Copy link
Contributor Author

PeterJFB commented Feb 19, 2023

Nice! 100 But why not just add a collapse prop on displayContent instead of making a nearly identical one?

Honest answer, I was lazy:P

Looking over the other pages, I didn't really think it was worth the time to make the more logic-intensive component. I also think this solution will have the least performance-impact, since it will not add the UseRef-code to components that don't need it.

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.

:shipit:

@ivarnakken ivarnakken added approved Pull requests that have been approved and removed changes-requested Pull requests with requested changes labels Feb 20, 2023
Copy link
Contributor

@Arashfa0301 Arashfa0301 left a comment

Choose a reason for hiding this comment

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

Looks good to me :)

Copy link
Member

@ingraso ingraso left a comment

Choose a reason for hiding this comment

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

Nice work! Looks very nice 🚀

@PeterJFB PeterJFB merged commit 5b722d6 into master Feb 21, 2023
@PeterJFB PeterJFB deleted the collapsable-description branch February 21, 2023 16:36
@PeterJFB PeterJFB removed the review-needed Pull requests that need review label Feb 21, 2023
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
Projects
None yet
5 participants