Skip to content

Conversation

alexandcote
Copy link
Contributor

@alexandcote alexandcote commented Mar 16, 2020

WHY are these changes introduced?

Fixes #742

The DropZone determines its size (height and width) based on width only. This makes certain valid use cases, where the default DropZone height isn't appropriate, impossible.

WHAT is this pull request doing?

Adding a new Dropzone wrapper that use the power of flexbox to determine the height.

🎩 checklist

  • Tested on mobile
  • Tested on multiple browsers
  • Tested for accessibility
  • Updated the component's README.md with documentation changes
  • Tophatted documentation changes in the style guide
  • For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

@github-actions
Copy link
Contributor

github-actions bot commented Mar 16, 2020

🟢 This pull request modifies 4 files and might impact 1 other files.

Details:
All files potentially affected (total: 1)
📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🎨 src/components/DropZone/DropZone.scss (total: 1)

Files potentially affected (total: 1)

🧩 src/components/DropZone/DropZone.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/DropZone/tests/DropZone.test.tsx (total: 0)

Files potentially affected (total: 0)

@alexandcote alexandcote force-pushed the dropzone_fixed_height branch from 7a259fb to 0bbc85f Compare March 16, 2020 15:58
@alexandcote alexandcote marked this pull request as ready for review March 16, 2020 15:58
@alexandcote alexandcote self-assigned this Mar 16, 2020
@alexandcote alexandcote force-pushed the dropzone_fixed_height branch 6 times, most recently from 82cddde to 3fc4945 Compare March 16, 2020 18:50
@alexandcote alexandcote force-pushed the dropzone_fixed_height branch from 3fc4945 to 8101489 Compare March 16, 2020 18:54
@dleroux dleroux self-requested a review March 17, 2020 12:54
Copy link
Contributor

@dleroux dleroux left a comment

Choose a reason for hiding this comment

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

I applied the changes to pass travis, top-hatted in the playground, in chrome and edge chromium. All looks good.

I didn't tophat in web, might be worth if someone has a chance to yarn build-consumer web

Copy link
Contributor

@beefchimi beefchimi 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! I tested this out in Storybook and I didn't see any issues. If possible, we should give this a try in online-store-web before merging.

It is interesting to see this solution. The previous WIP PR seemed rather complicated - maybe I missed something on my initial scan, but are the two PRs achieving the same thing @alexandcote ? Or are you intentionally leaving out some complexity?

.DropZoneWrapper {
height: 100%;

> div {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is always a bit risky to do, because if the Labelled wrapping markup changes we need to make sure that this will change as well. I think thanks to the splash build tooling that polaris uses, we will get a notification that at least indicates DropZone will be affected by Labelled changes. Still... I'm wondering if we should qualify this with a comment to express why we need to override some Labelled styles?

Copy link
Member

Choose a reason for hiding this comment

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

Probably a comment or it'll be forgotten about and list in the 💨

@AndrewMusgrave
Copy link
Member

@maxariss I remember this originally broke something you were working on, but I can't remember exactly where the dropzone was. Mind giving me a reminder so we can 🎩 in that area?

@AndrewMusgrave
Copy link
Member

cc/ @maxariss

1 similar comment
@AndrewMusgrave
Copy link
Member

cc/ @maxariss

@maxariss
Copy link
Contributor

maxariss commented Apr 9, 2020

@AndrewMusgrave I could be mistaken, but I believe one area where this issue was originally raised was the ImageModal on the VariantDetails page. I think we were trying to use the dropzone as a grid item within the image grid. However, It looks like at some point someone modified the component to just make the entire card a Dropzone, and then custom baked the grid item to appear like a dropzone (added a dashed border and some other styles to mimic Dropzone).

Screen Shot 2020-04-09 at 11 17 14 AM

Screen Shot 2020-04-09 at 11 17 36 AM

@AndrewMusgrave AndrewMusgrave removed their request for review June 12, 2020 18:36
@dhmacs
Copy link

dhmacs commented Aug 5, 2020

Any update on this one?

@AndrewMusgrave
Copy link
Member

cc/ @alexandcote

@alexandcote
Copy link
Contributor Author

No update on my side, I opened this PR in March but now I don't have time to take a second look. Feel free to update/merge/close the PR if anyone had time.

@alex-page alex-page force-pushed the master branch 2 times, most recently from 4eab9e5 to 9e09ba7 Compare January 15, 2021 05:17
Base automatically changed from master to main January 21, 2021 15:38
@alex-page alex-page force-pushed the main branch 7 times, most recently from 2c6e842 to d2611d6 Compare June 11, 2021 16:24
@alex-page alex-page force-pushed the main branch 5 times, most recently from c630336 to ea3c495 Compare March 24, 2022 22:41
@alex-page
Copy link
Member

@alexandcote is this still an issue?

@alex-page alex-page closed this May 19, 2022
@alex-page alex-page deleted the dropzone_fixed_height branch May 19, 2022 23:07
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.

DropZone height is not respected

7 participants