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

UI – Calendar events modal follow up #17788

Merged
merged 8 commits into from
Mar 22, 2024

Conversation

jacobshandling
Copy link
Contributor

@jacobshandling jacobshandling commented Mar 22, 2024

Follow-up work to #17717 for #17445

Finalize disabled options and tooltips:
Screenshot 2024-03-21 at 5 14 40 PM
Screenshot 2024-03-21 at 5 15 13 PM

Only update policies and settings when there's a diff:
1(1)

Reorganize onChange handlers, types

  • Manual QA for all new/changed functionality

@jacobshandling jacobshandling requested a review from a team as a code owner March 22, 2024 00:40
Copy link

codecov bot commented Mar 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.24%. Comparing base (268dd18) to head (955a098).
Report is 3 commits behind head on 17230-calendar-feature.

Additional details and impacted files
@@                   Coverage Diff                   @@
##           17230-calendar-feature   #17788   +/-   ##
=======================================================
  Coverage                   65.24%   65.24%           
=======================================================
  Files                        1202     1202           
  Lines                      109371   109371           
  Branches                     2587     2587           
=======================================================
  Hits                        71358    71358           
  Misses                      32598    32598           
  Partials                     5415     5415           
Flag Coverage Δ
frontend 51.53% <ø> (ø)

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.

teamPoliciesAPI.update(formPolicy.id, {
calendar_events_enabled: formPolicy.isChecked,
// update changed policies calendar events enabled
const changedPolicies = formData.policies.filter((formPolicy) => {
Copy link
Member

Choose a reason for hiding this comment

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

nice

Copy link
Member

Choose a reason for hiding this comment

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

👏

@@ -23,8 +23,21 @@
width: 360px;
.is-disabled * {
color: $ui-fleet-black-25;
.label-text {
font-style: normal;
// increase height to allow for broader tooltip activation area
Copy link
Member

Choose a reason for hiding this comment

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

love it

@@ -157,7 +150,7 @@ const CalendarEventsModal = ({
name={name}
// can't use parseTarget as value needs to be set to !currentValue
onChange={() => {
onInputChange({ name, value: !isChecked });
onPolicyEnabledChange({ name, value: !isChecked });
Copy link
Member

Choose a reason for hiding this comment

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

🔥

Copy link
Member

@RachelElysia RachelElysia left a comment

Choose a reason for hiding this comment

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

Sweet, looks super clean. Love the tooltip implementation and some of the naming updates!!

Just a few styling nits, mostly color, it should be quick fixes:

  1. Dropdown should be blue
Screenshot 2024-03-22 at 10 52 12 AM 2. Description text of a selectable dropdown option should be grey Screenshot 2024-03-22 at 10 52 22 AM 3. This tooltip should be a dark blue, not black Screenshot 2024-03-22 at 10 55 11 AM 4. Need to change padding of the code blocks to be smaller Screenshot 2024-03-22 at 10 55 59 AM 5. Question for @rachaelshaw : Are we moving away from punctuation in single sentence flash messages? If so, we need to remove the periods from the success and fail flash messages to match Figma. Screenshot 2024-03-22 at 10 57 51 AM

@jacobshandling
Copy link
Contributor Author

jacobshandling commented Mar 22, 2024

@RachelElysia thanks

regarding 3 – I've seen some strange behavior here. I saw that, then refreshed, and it was gone, and figured it was just a matter of loading new stylesheets. But then saw it again, and refreshed, and it went away again. I'll look into it, but I imagine if you hard refresh it should be the right colors.

Regarding 4., I assumed the Figma was inaccurate here - @rachaelshaw do we want to keep the same padding as for other code blocks like this, or update it with less padding per current Figma?

@RachelElysia
Copy link
Member

@jacobshandling Ah yeah, regarding 4. I made my padding smaller on my other workflows modal, so definitely want to hear back from @rachaelshaw so we are consistent app-wide.

@rachaelshaw
Copy link
Member

rachaelshaw commented Mar 22, 2024

@RachelElysia @jacobshandling we should keep the padding consistent with other code blocks in the UI instead of having it different for just this page. If you let me know what the typical padding is, I'll update the Figma Just updated the Figma to have existing 24px padding (I started with a code block pasted in from an older file, so didn't actually double-check the UI for that one).

@jacobshandling
Copy link
Contributor Author

Thanks @rachaelshaw, that just leaves the flash message copy decisions. Currently in this PR, the messages match those of the "Other workflows" modal, the other dropdown option. If you'd like us to update those msssages (should probably be for both of these modals) to those in the Figma, we can do that too.

@jacobshandling jacobshandling merged commit fbf836a into 17230-calendar-feature Mar 22, 2024
10 checks passed
@jacobshandling jacobshandling deleted the cal-events-modal-follow-up branch March 22, 2024 18:54
@rachaelshaw
Copy link
Member

that just leaves the flash message copy decisions

Should have a period to be consistent; updated Figma.

getvictor pushed a commit that referenced this pull request Mar 25, 2024
## Follow-up work to #17717 

**Finalize disabled options and tooltips:**
<img width="697" alt="Screenshot 2024-03-21 at 5 14 40 PM"
src="https://github.com/fleetdm/fleet/assets/61553566/ea5d880f-75f6-48ef-85cc-b807812c9a50">
<img width="697" alt="Screenshot 2024-03-21 at 5 15 13 PM"
src="https://github.com/fleetdm/fleet/assets/61553566/bdd33118-933e-4676-9e1e-680ebcddbc7a">

**Only update policies and settings when there's a diff:**

![1(1)](https://github.com/fleetdm/fleet/assets/61553566/183d1834-3c54-4fef-a208-dfbb0354e507)

**Reorganize onChange handlers, types**

- [x] Manual QA for all new/changed functionality

---------

Co-authored-by: Jacob Shandling <jacob@fleetdm.com>
getvictor pushed a commit that referenced this pull request Mar 26, 2024
## Follow-up work to #17717 

**Finalize disabled options and tooltips:**
<img width="697" alt="Screenshot 2024-03-21 at 5 14 40 PM"
src="https://github.com/fleetdm/fleet/assets/61553566/ea5d880f-75f6-48ef-85cc-b807812c9a50">
<img width="697" alt="Screenshot 2024-03-21 at 5 15 13 PM"
src="https://github.com/fleetdm/fleet/assets/61553566/bdd33118-933e-4676-9e1e-680ebcddbc7a">

**Only update policies and settings when there's a diff:**

![1(1)](https://github.com/fleetdm/fleet/assets/61553566/183d1834-3c54-4fef-a208-dfbb0354e507)

**Reorganize onChange handlers, types**

- [x] Manual QA for all new/changed functionality

---------

Co-authored-by: Jacob Shandling <jacob@fleetdm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants