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.

Sign up for GitHub

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

Jump to bottom

Visually distinguish invitations in calendar grid #6624

Merged
merged 1 commit into from
Feb 17, 2025
Merged

Visually distinguish invitations in calendar grid #6624

merged 1 commit into from
Feb 17, 2025
+163 −4

Conversation

GVodyanov
Copy link
Contributor

@GVodyanov GVodyanov commented Jan 13, 2025 •
edited
Loading

Fix #3869

swappy-20250121_193855

Based on #3869 (comment)

@GVodyanov GVodyanov added the 2. developing Work in progress label Jan 13, 2025
@GVodyanov GVodyanov self-assigned this Jan 13, 2025
@codecov Codecov
Copy link

codecov bot commented Jan 13, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 14.70588% with 58 lines in your changes missing coverage. Please review.

Project coverage is 22.99%. Comparing base (7b81283) to head (a68f90c).
Report is 23 commits behind head on main.

Files with missing lines Patch % Lines
src/fullcalendar/rendering/eventDidMount.js 12.24% 37 Missing and 6 partials ⚠️
...c/fullcalendar/eventSources/eventSourceFunction.js 21.05% 8 Missing and 7 partials ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #6624      +/-   ##
============================================
- Coverage     23.02%   22.99%   -0.03%     
  Complexity      475      475              
============================================
  Files           252      252              
  Lines         12101    12171      +70     
  Branches       2307     2328      +21     
============================================
+ Hits           2786     2799      +13     
- Misses         8989     9033      +44     
- Partials        326      339      +13
Flag Coverage Δ
javascript 14.58% <14.70%> (+0.02%) ⬆️
php 59.30% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@GVodyanov GVodyanov force-pushed the style/conditional-styling-attendee-status branch from 0916f30 to d340b11 Compare January 21, 2025 18:50
@GVodyanov GVodyanov marked this pull request as ready for review January 21, 2025 18:50
@GVodyanov GVodyanov added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jan 21, 2025
@nimishavijay
Copy link
Member

Looking great! Could you also post a screenshot in light mode, and one with the entire screen? to check the contrast.
So far I have only one suggestion, which is to use the filled warning symbol instead of the outlined one so that it's visible more clearly.

@GVodyanov
Copy link
Contributor Author

GVodyanov commented Jan 23, 2025 •
edited
Loading

Looking great! Could you also post a screenshot in light mode, and one with the entire screen? to check the contrast.

Here you go @nimishavijay

image

The outlined yellow event on a white background is the worst we can get in terms of contrast (at least out of the default colors)... I agree it's not great, I guess the spec could be changed to either modify the text and have it be the normal text color, or leave the background and have some other way of showing it's declined, what do you think is best?

PS I'll change the icon now

@nimishavijay
Copy link
Member

nimishavijay commented Jan 23, 2025 •
edited by GVodyanov
Loading

Yep, exactly what I thought too.

  • We can definitely bold the text for full day events, considering the regular events are bold too.

  • To be on the safer side I'd also agree that we can make it the normal text color.

  • Other than that I can only think of maybe using a thicker border (maybe 2px?). We can try that and see, let me know what you think.

  • Also unrelated nitpick but I noticed that today's events are like 1px too low (possibly because of the box around the date). Is it possible to fix that?

@GVodyanov
Copy link
Contributor Author

GVodyanov commented Jan 23, 2025 •
edited
Loading

First things first, here's the new icon

image

@GVodyanov
Copy link
Contributor Author

How is this looking? @nimishavijay

image

image

@SebastianKrupinski
Copy link
Contributor

Looking good! 👍

@nimishavijay
Copy link
Member

nimishavijay commented Jan 27, 2025 •
edited by GVodyanov
Loading

Awesome! Thanks for the comprehensive screenshots. 2px works! Looks much clearer now. Couple of more nitpicks:

  • I see that the height of the needs-action and confirmed full day events are different (needs-action is slightly more because of the border). Could we make this consistent? Maybe by using outline instead of border or decreasing the height and adding a border to the confirmed event in the same colour, you get the idea
  • The outlined circle stroke also seems very thin, possible to make that also 2px? (Also make sure to keep the size consistent with the filled circle like the previous point)
  • add 2px spacing between the warning icon and the text, rn it seems very close

Other than that it is ready to go! Super nice work! :)

@st3iny
Copy link
Member

st3iny commented Jan 27, 2025

/backport to stable5.1

@backportbot backportbot bot added the backport-request A backport was requested for this pull request label Jan 27, 2025
@st3iny st3iny added this to the v5.2.0 milestone Jan 27, 2025
@GVodyanov
Copy link
Contributor Author

You have a super attentive eye @nimishavijay! Thanks for the review, how's it looking now?

(zoomed to see alignment)

image

nimishavijay
nimishavijay approved these changes Jan 31, 2025
View reviewed changes
Copy link
Member

@nimishavijay nimishavijay left a comment

Choose a reason for hiding this comment

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

Awesome! Looks perfect now!

@GVodyanov GVodyanov force-pushed the style/conditional-styling-attendee-status branch from 679c25d to ccb02fe Compare February 2, 2025 17:18
st3iny
st3iny requested changes Feb 4, 2025
View reviewed changes
Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

Left some feedback. Eslint is not yet happy.

@GVodyanov GVodyanov force-pushed the style/conditional-styling-attendee-status branch from ccb02fe to 206838c Compare February 9, 2025 20:15
@GVodyanov
Copy link
Contributor Author

The tests also seem to be failing because of the usage of Pinia outside of a vue component, I think that the tests have to be modified, but I will check if that is the best way to solve this.

@st3iny st3iny force-pushed the style/conditional-styling-attendee-status branch from 206838c to e43b528 Compare February 17, 2025 16:48
st3iny
st3iny approved these changes Feb 17, 2025
View reviewed changes
Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

Tested and works. Great job, this is a neat improvement!

@GVodyanov @st3iny
feat(FullCalendar): add conditional styling for participation status …
a68f90c 
…in grid

Signed-off-by: Grigory Vodyanov <scratchx@gmx.com>
Signed-off-by: Richard Steinmetz <richard@steinmetz.cloud>
@st3iny st3iny force-pushed the style/conditional-styling-attendee-status branch from e43b528 to a68f90c Compare February 17, 2025 16:51
@st3iny st3iny added design Related to design, interface, interaction design, UX, etc. enhancement New feature request Feature: Fullcalendar 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Feb 17, 2025
@st3iny st3iny enabled auto-merge February 17, 2025 16:52
@st3iny st3iny merged commit 43a7530 into main Feb 17, 2025
46 of 48 checks passed
@st3iny st3iny deleted the style/conditional-styling-attendee-status branch February 17, 2025 16:55
@backportbot backportbot bot mentioned this pull request Feb 17, 2025
Closed
@backportbot backportbot bot removed the backport-request A backport was requested for this pull request label Feb 17, 2025
@SebastianKrupinski
Copy link
Contributor

@GVodyanov

How difficult would it be to add the highlighting recommendation to this PR? from this ticket.

#6708

@SebastianKrupinski
Copy link
Contributor

SebastianKrupinski commented Feb 17, 2025 •
edited
Loading

Well NVM, @st3iny merged it as I posted the comment 😆

@nimishavijay nimishavijay mentioned this pull request Feb 18, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@st3iny st3iny st3iny approved these changes

@nimishavijay nimishavijay nimishavijay approved these changes

@GretaD GretaD Awaiting requested review from GretaD

@SebastianKrupinski SebastianKrupinski Awaiting requested review from SebastianKrupinski SebastianKrupinski is a code owner

@tcitworld tcitworld Awaiting requested review from tcitworld tcitworld is a code owner

Assignees

@GVodyanov GVodyanov

Labels
4. to release Ready to be released and/or waiting for tests to finish design Related to design, interface, interaction design, UX, etc. enhancement New feature request Feature: Fullcalendar
Projects
None yet
Milestone
v5.2.0
Development

Successfully merging this pull request may close these issues.

Visually distinguish invitations
4 participants
@GVodyanov @nimishavijay @SebastianKrupinski @st3iny