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

📅 Calendar view #2878

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from
Draft

📅 Calendar view #2878

wants to merge 16 commits into from

Conversation

yshmarov
Copy link

@yshmarov yshmarov commented Jun 20, 2024

Demo

  • open calendar (month) view
  • highlight current day
  • hover a calendar record
  • navigate to another period (month) <- ->
  • open a calendar record
  • clicking "back" from the #show page redirects to the correct calendar page
  • clicking Today opens current month

avo-calendar-gic

Description

Calendar view. Based on #2727

Monthly calendar currently implemented. We can consider other resolutions in the future.

Records are displayed in calendar by default by created_at, but this can be overriden in the resource with self.calendar_view

Date is stored in params => refreshing page will not lose current period

TODO:

  • styling
  • week, day, month view? (not just "calendar"?)
  • use a better calendar icon
  • tests
  • event duration
  • user can select starts_at attribute
  • user can format title
  • group events that start at the same time

@adrianthedev 's requirements:

  • a read-only view of events
  • events shown in order on a day cell (if they have a time)
  • events that can span over a longer period of time - in the first iteration I do not want to include duration

Fixes # (issue)

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

Screenshots & recording

Manual review steps

  1. Step 1
  2. Step 2

Manual reviewer: please leave a comment with output from the test if that's the case.

Copy link

codeclimate bot commented Jun 20, 2024

Code Climate has analyzed commit 8d6b57e and detected 0 issues on this pull request.

View more on Code Climate.

this way it will be easier to add duration with "ends_at"
currently avo resets view filter when navigating back. we can persist it for calendar and other views.

we can also consider persisting other params
if no title is set, defaults to ID
not just select an attribute. defaults to created_at time
@admsev
Copy link

admsev commented Jun 25, 2024

@gooroodev review

@gooroodev
Copy link

Thanks for the call-out, @admsev!

🐞Mistake 🤪Typo 🚨Security 🚀Performance 💪Best Practices 📖Readability ❓Others
1 0 0 0 2 2 0

Summary of Proposed Changes

  • ➕ Added a new calendar view type for resources.
  • ➕ Introduced a new SVG icon for the calendar view type.
  • ➕ Created ResourceCalendarComponent for rendering calendar views.
  • ➕ Updated ResourceIndexComponent to support rendering the new calendar view.
  • ➕ Modified ResourceShowComponent to maintain the calendar view state when navigating back.
  • ➕ Extended the view toggle button to include the calendar view.
  • ➕ Updated Post resource example to support the new calendar view.

Identified Issues

ID Type Details Severity Confidence
1 🐞Mistake Missing class name in div tag in resource_index_component.html.erb (line 89). 🟠Medium 🟠Medium
2 💪Best Practices Hardcoded date formats in resource_calendar_component.html.erb should use localization. 🟠Medium 🟠Medium
3 💪Best Practices rotate method in resource_calendar_component.html.erb can be simplified for readability. 🟠Medium 🟠Medium
4 📖Readability Inline CSS classes in resource_calendar_component.html.erb reduce readability. 🟠Medium 🟠Medium

Issue 1

ID: 1

Explanation:
In app/components/avo/views/resource_index_component.html.erb, the div tag on line 89 is missing a closing quote for the class attribute.

Code:

<div id="records_panel mt-6">

Fix:

<div id="records_panel" class="mt-6">

Explanation:
This fix ensures that the div tag has a properly formatted class attribute.

Issue 2

ID: 2

Explanation:
Hardcoded date formats in resource_calendar_component.html.erb should use localization to support different locales.

Code:

<p class="font-bold text-xl">
  <%= date.strftime('%B %Y') %>
</p>

Fix:

<p class="font-bold text-xl">
  <%= l(date, format: :calendar_header) %>
</p>

Explanation:
This fix uses Rails' localization to format dates, making the application more flexible for different locales.

Issue 3

ID: 3

Explanation:
The rotate method in resource_calendar_component.html.erb can be simplified for better readability.

Code:

<% Date::ABBR_DAYNAMES.rotate.each do |day| %>

Fix:

<% Date::ABBR_DAYNAMES.each do |day| %>

Explanation:
Since the rotate method is not necessary here, removing it simplifies the code.

Issue 4

ID: 4

Explanation:
Inline CSS classes in resource_calendar_component.html.erb reduce readability and maintainability.

Code:

<div class="border border-gray-100 px-0.5 min-h-24 <%= today_class(day) %>">

Fix:

<div class="calendar-day <%= today_class(day) %>">

Explanation:
This fix moves the CSS classes to a dedicated CSS file, improving readability and maintainability.

General Review

The code quality and style are generally good, but there are a few areas that could be improved for better readability and maintainability. Specifically, consider using localization for date formats and avoid inline CSS classes. Additionally, ensure that all HTML attributes are properly formatted to avoid rendering issues.

--
I only arrive when I am mentioned and asked to review the pull request.
React or reply to give your feedback!

@Paul-Bob
Copy link
Contributor

Paul-Bob commented Jul 3, 2024

Oups

@Paul-Bob Paul-Bob reopened this Jul 3, 2024
@yshmarov
Copy link
Author

yshmarov commented Jul 3, 2024

heyyyy

@yshmarov
Copy link
Author

yshmarov commented Jul 3, 2024

Questions:

  1. should user be able to somehow select default start of week? (sunday/monday)? maybe not for version 1?
  2. @admsev can you help me find a better calendar icon, that looks organic next to the table and grid icons?
  3. is it OK to go with :calendar, or should we go with :month? If we go with :month, we can add :day/:week resolutions in the same line. I would go with this!
  4. should I add some sort of tests?
  5. should I add changes to the docs? can you please recommend what file to add the doc changes to?

@yshmarov yshmarov changed the title Calendar view: second try 📅 Calendar view Jul 3, 2024
@yshmarov
Copy link
Author

yshmarov commented Jul 4, 2024

avo-calendar.mp4

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.

None yet

5 participants