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

Introduce "too late" presence option to event attendees #4528

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

ivarnakken
Copy link
Member

Description

Related backend changes

Neither ion-icons or font-awesome had a turtle icon (which I thought was the most fitting), so I extracted one from Lucide (which we're planning on migrating to). I'm very open to design suggestions, as I'm not sure what I feel about this. The padding feels a bit much, so I might refactor the Icon component - but that will probably have to wait.

Result

If you've made visual changes, please check the boxes below and include images showing the changes. Descriptions are appreciated.

  • Changes look good on both light and dark theme.
  • Changes look good with different viewports (mobile, tablet, etc.).
  • Changes look good with slower Internet connections.
Description Before After
The icons are much thinner and less "visible", but at the same time they are now Ionicons and therefore match all our other icons - so in that sense they are more "correct". image image

The automatic penalty:

image

Testing

  • I have thoroughly tested my changes.

Things work. Tested that all other icons that use the TooltipIcon component still work and look good.

@ivarnakken ivarnakken added review-needed Pull requests that need review new-feature Pull requests that introduce a new feature labels Mar 7, 2024
@ivarnakken ivarnakken requested a review from a team March 7, 2024 10:40
@ivarnakken ivarnakken self-assigned this Mar 7, 2024
Copy link

linear bot commented Mar 7, 2024

Copy link
Member

@norbye norbye left a comment

Choose a reason for hiding this comment

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

Awesome, love the look!

While the padding may feel like much - I currently like how it is evenly spaced vertically and horizontally. And anyways lets get it out there to get a feel of it

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.

Awesome!
As for the design, it might also work well with a dropdown like in the bdb

app/models.ts Outdated Show resolved Hide resolved
@ivarnakken
Copy link
Member Author

As for the design, it might also work well with a dropdown like in the bdb

I contemplated it. Even though it would save us space, it would require the user to click two times, instead of just one, so I ended up not doing it.

@ivarnakken ivarnakken added approved Pull requests that have been approved changes-requested Pull requests with requested changes labels Mar 8, 2024
@ivarnakken ivarnakken removed the changes-requested Pull requests with requested changes label Mar 9, 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.

Looks good!

Neither ion-icons or font-awesome had a turtle icon, so I extracted one
from Lucide (which we're planning on migrating to). I'm very open to
design suggestions, as I'm not sure what I feel about this. The padding
feels a bit much, so I might refactor the Icon component - but that will
probably have to wait.
@ivarnakken ivarnakken merged commit 0c60ed3 into master Mar 15, 2024
4 checks passed
@ivarnakken ivarnakken deleted the ivarnakken/aba-868-too-late branch March 15, 2024 20:09
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 new-feature Pull requests that introduce a new feature review-needed Pull requests that need review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants