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

refactor: improve calendar performance and style #518

Open
wants to merge 24 commits into
base: develop
Choose a base branch
from

Conversation

nunom27
Copy link
Contributor

@nunom27 nunom27 commented Aug 8, 2024

@nunom27 nunom27 self-assigned this Aug 8, 2024
@nunom27 nunom27 marked this pull request as ready for review August 26, 2024 23:31
Copy link
Member

@ruilopesm ruilopesm left a comment

Choose a reason for hiding this comment

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

Week view, in mobile, is not showing events, for some reason 🤔

@nunom27
Copy link
Contributor Author

nunom27 commented Aug 29, 2024

Week view, in mobile, is not showing events, for some reason 🤔

Do you actually follow the organization? You can try unfollowing and following the organization that has the activity again. I believe the seeds that makes generated users follow organizations is a bit broken, because the button shows up as following when the organization has 0 followers.

Also, does the same activity show up in any other view? For me it seems to be working.

@ruilopesm
Copy link
Member

@nunom27 I can see activities properly while on a desktop dimensions. After resizing the screen to mobile, activities do not show anymore.

I guess we'll have to wait until someone try to reproduce it 😅

@joaodiaslobo
Copy link
Member

joaodiaslobo commented Sep 1, 2024

Can't reproduce it on my side @ruilopesm . 🤔
On top of the mobile week view, make sure to select a day where there are events! (in this example, day 7 is already selected)

2024-09-01.08-29-40.mp4

@MarioRodrigues10
Copy link
Member

It works on my side, but looks a little bit odd on my side.
image
image

@nunom27
Copy link
Contributor Author

nunom27 commented Sep 2, 2024

@MarioRodrigues10 That happened due to the activity starting at 23:35, so it only takes 5 rows until midnight (each row represents 5 minutes intervals). I can try, however, to put the title right in front of the time on those cases, so that it doesn't look as odd.

@ruilopesm
Copy link
Member

It's properly working for me now. I might have tested it wrongly 😅

lib/atomic_web/components/calendar/month.ex Outdated Show resolved Hide resolved
lib/atomic_web/components/calendar/month.ex Outdated Show resolved Hide resolved
lib/atomic_web/components/calendar/month.ex Outdated Show resolved Hide resolved
lib/atomic_web/components/calendar/month.ex Outdated Show resolved Hide resolved
Comment on lines 143 to 149
<%= if Enum.count(get_date_activities(@activities, @date)) > 2 do %>
<li class="text-zinc-500 hover:text-primary-600">
<button type="button" phx-click="show-more" phx-value-date={@date}>
+<%= Enum.count(get_date_activities(@activities, @date)) - 2 %> more
</button>
</li>
<% end %>
Copy link
Member

Choose a reason for hiding this comment

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

There's a new short syntax for cases like this (same for other places, if you prefer it like so)

<li :if={Enum.count(get_date_activities(@activities, @date)) > 2} class="...">

Also, would you consider this calculation heavy? If so, prefer to assign it before rendering the template, by calculating it only once (you are using it at least twice).

lib/atomic_web/components/calendar/month.ex Outdated Show resolved Hide resolved
lib/atomic_web/components/calendar/week.ex Outdated Show resolved Hide resolved
lib/atomic_web/components/calendar/week.ex Outdated Show resolved Hide resolved
lib/atomic_web/live/calendar_live/show.html.heex Outdated Show resolved Hide resolved
lib/atomic_web/views/calendar_utils.ex Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants