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

feat(core): created a new legend section #12915

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alexhristov14
Copy link
Contributor

closes #12403

Copy link

netlify bot commented Jan 10, 2025

Check the deploy log for errors here: https://app.netlify.com/sites/fundamental-ngx/deploys

Name Link
🔨 Latest commit 44ca71c
🔍 Latest deploy log https://app.netlify.com/sites/fundamental-ngx/deploys/67896ac7298e920008fb2508

Copy link

github-actions bot commented Jan 10, 2025

Visit the preview URL for this PR (updated for commit 44ca71c):

https://fundamental-ngx-gh--pr12915-feat-calendar-legend-qfybq5e1.web.app

(expires Mon, 27 Jan 2025 18:46:21 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 41b993ee8e451bd7c6770b342ce142dc886eacff

Copy link
Member

@mikerodonnell89 mikerodonnell89 left a comment

Choose a reason for hiding this comment

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

This is looking good, but I left a few comments on things to consider. Also this PR seems to just add this to calendar, I think to consider the original issue completed, we'll need to add this to datepicker/datetimepicker as well. Though I would be in favor of considering that a separate task, I prefer several small pr's over one large one when possible.

@@ -58,7 +58,7 @@
"fast-deep-equal": "3.1.3",
"focus-trap": "7.1.0",
"focus-visible": "5.2.1",
"fundamental-styles": "0.38.0",
"fundamental-styles": "0.39.0-rc.22",
Copy link
Member

Choose a reason for hiding this comment

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

need to release a new fundamental-styles version and update here before merging this pr


/** @hidden */
ngOnInit(): void {
this._addCalendarLegend();
Copy link
Member

Choose a reason for hiding this comment

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

This logic being placed in the init lifecycle method will lead to problems if the special days are updated after the calendar is created. You could watch for changes to the specialDaysRules. But I think a better way of handling this would be to avoid typescript-side logic wherever possible, instead opting to render as much as possible with double curly brace interpolation in calendar.component.html

added a few inputs to legend item

minor classes changes

used BuildClass

can insert a legend programatically with SpecialDays

made programmable through SpecialDayRules

adding a focusing service to filter legend days

modified the calendar legend docs

connected the legend to days, can be logged

remove unfocused classes

calendar cells focus/unfocus dynamically

legends are only focusing on their own calendar and not others

some written tests

calendar legend test passing

finished the tests for legend item

finished last test and made couple changes to docs

added path to index.ts
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.

display calendar legend
2 participants