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

change: [M3-7182] - Update RemovableSelectionsList default maximum height to cut off the last list item #9827

Merged
merged 5 commits into from
Oct 26, 2023

Conversation

coliu-akamai
Copy link
Contributor

@coliu-akamai coliu-akamai commented Oct 23, 2023

Description 📝

  • Add dropshadow and update maxHeight to list to indicate scrolling is necessary
  • Fix dropshadow UI bug on Safari for market place apps

some background

Preview 📷

Before After
image image

How to test 🧪

Prerequisites

(How to setup test environment)

  • checkout this branch

  • yarn storybook --> RemovableSelectionsList

  • Marketplace fix: navigate to the marketplace apps on safari

Verification steps

  • For the removable selections list

    • verify that the default example now cuts off the 11th item in the list.
    • verify that there is a drop shadow if the list can be scrolled
    • Alternatively, if you want to test this with the SubnetAssignLinodesDrawer, make sure you're on the dev environment with the vpc tags, and you can assign 11+ linodes to a subnet
  • for the Marketplace apps, verify that the drop shadow appears above the individual apps (it should now mirror chrome's behavior)

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@coliu-akamai coliu-akamai self-assigned this Oct 23, 2023
@coliu-akamai coliu-akamai marked this pull request as ready for review October 23, 2023 20:01
@coliu-akamai coliu-akamai requested a review from a team as a code owner October 23, 2023 20:01
@coliu-akamai coliu-akamai requested review from bnussman-akamai, tyler-akamai and abailly-akamai and removed request for a team October 23, 2023 20:01
@coliu-akamai coliu-akamai added Ready for Review VPC Relating to VPC project labels Oct 23, 2023
Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Looks good! Did we consider adding a inner shadow such as the marketplace list one?

It would prob add visual consistency with to those scrollable containers

Screenshot 2023-10-24 at 09 22 36

@coliu-akamai
Copy link
Contributor Author

@abailly-akamai thanks Alban! I'd checked with Andrew, and as of right now we do not plan to add the drop shadow. I'll update the internal ticket to make note of that!

@abailly-akamai
Copy link
Contributor

@coliu-akamai what was the reasoning? It's good to remember we may all have different experiences based on our browser settings

@tyler-akamai
Copy link
Contributor

Confirmed the list does now get cut off:
Screenshot 2023-10-24 at 9 55 39 AM

@tyler-akamai
Copy link
Contributor

tyler-akamai commented Oct 24, 2023

@coliu-akamai what was the reasoning? It's good to remember we may all have different experiences based on our browser settings

It might be worth taking a screenshot with and without the shadow, and then showing it to Andrew.

Personally, I do like how the shadow looks but I also have to see it in this application.

@coliu-akamai
Copy link
Contributor Author

coliu-akamai commented Oct 24, 2023

@abailly-akamai let me double check, but iirc, we saw behavior with the drop shadow disappearing behind the list items in the marketplace apps when scrolling that was weird and potentially even made it worth revisiting the shadow there. However, I'm currently not seeing that on my browser (chrome) for marketplace and the lists, so I'll check with Andrew again and get back to you.

edit: the weird behavior was on Safari

@tyler-akamai @abailly-akamai I'm experimenting with the drop-shadow for the react list, below (if we go with the drop shadow, we may need to darken the color due to the background for this list being darker than the marketplace app's background -- currently using theme.color.boxShadow).

Am also currently looking into a way to get the drop shadow to only show up if the list needs a scrollbar:

image
image

@coliu-akamai
Copy link
Contributor Author

@abailly-akamai to follow up on this, spoke with Andrew - it is Safari that has the weird behavior with box shadows (note how the market place apps are above the boxshadow when scrolling).

image

Andrew noted that this may be how Safari treats boxshadows, and that it might be better to use a gradient instead --I'm going to look more into this issue soon.

Regarding the box shadow on the RemovableSelectionsList - got the go ahead to add it for when the list actually needs to scroll (but otherwise hide it if we do not yet need to scroll the list). Just pushed up that change!

@abailly-akamai
Copy link
Contributor

@coliu-akamai great thanks for the update! i wonder if the safari bug has to do with a simple z-index issue.

@tyler-akamai
Copy link
Contributor

tyler-akamai commented Oct 25, 2023

  • Shadow appears with enough items to scroll
    Screenshot 2023-10-25 at 10 02 53 AM

  • Shadow does not appear when there is not enough items to scroll

10 items 9 items
Screenshot 2023-10-25 at 10 03 29 AM Screenshot 2023-10-25 at 10 04 16 AM

When there is 10 items the shadow appears even though its not scrollable. But as soon as you go down to 9, the shadow disappears. This is a very small detail but just throwing it out there.

Also, I love the Reset List button, makes testing really easy.

@coliu-akamai
Copy link
Contributor Author

Thanks @tyler-akamai! Actually gonna make a few changes to make the drop shadow more dynamic, will lyk when those changes are pushed!

Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

Safari Marketplace UI is fixed, VPC changes look as described

@tyler-akamai
Copy link
Contributor

Looks and works great, great job!

@mjac0bs mjac0bs added Approved Multiple approvals and ready to merge! and removed Ready for Review labels Oct 25, 2023
Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Looks great! thanks for fixes.

confirmed max-height and dynamic list height logic ✅
Confirmed drop shadow fixes in both places ✅

@coliu-akamai coliu-akamai merged commit 55b1205 into linode:develop Oct 26, 2023
11 checks passed
@coliu-akamai coliu-akamai deleted the feat-m3-7182 branch November 6, 2023 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! VPC Relating to VPC project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants