feat(file-input): new module #2435

kentokage · 2024-09-03T18:14:40Z

Fixes # #2428

This PR contains CSS changes
This PR does not contain CSS changes

Description

This is the first iteration (alpha) of file-upload. Two building block components created file-preview-card and file-preview-card-group to support the display of uploaded assets in card form (list will be done later). This PR focuses on structure, a11y, and css.

Notes

Screenshots

file-upload

// base

// rtl

file-preview-card

// uploading

// images

// video

// docs

// overflow

file-preview-card-group

// base

// Mixed types with details (details artificially inserted and is not part of the file-preview-card)

// RTL

// 512 container

// 768 container

See skin app and storybook for more responsive breakpoints

Checklist

I verify the build is in a non-broken state
I verify all changes are within scope of the linked issue

I regenerated all CSS files under dist folder
I tested the UI in all supported browsers
I did a visual regression check of the components impacted by doing a Percy build and approved the build
I tested the UI in dark mode and RTL mode
I added/updated/removed Storybook coverage as appropriate

fix(file-upload): update css feat(file-upload): fix css feat(file-upload): add modules feat(file-upload): fix module grammar

changeset-bot · 2024-09-03T18:14:44Z

🦋 Changeset detected

Latest commit: 9add60c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@ebay/skin	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

LuLaValva

Thanks for the quick work @kentokage! I started with a few comments, but before I go further with the nitpicks I have a few high-level questions:

Is the --large modifier necessary, or should it be based on a media query? In the designs it looks like cards are supposed to be large on small screens.
Width of cards in the spec is labeled as max width and not width in general. My interpretation is that for small width screens, width should be adjusted so that two cards always fit cleanly in each row (even when that means decreasing card size):
Should cards be flush with the right side of the container, instead of moving to a newline whenever they don't fit? It's a little frustrating since all designs have "perfect" screen sizes, but I think it may make more sense to use space-between instead of forcing gap to be 32px as labeled in the design.

src/sass/card/card.scss

src/sass/card/stories/card-large.stories.js

src/sass/file-upload/file-upload.scss

kentokage · 2024-09-04T01:31:00Z

Thanks for the quick work @kentokage! I started with a few comments, but before I go further with the nitpicks I have a few high-level questions:

Is the --large modifier necessary, or should it be based on a media query? In the designs it looks like cards are supposed to be large on small screens.

Width of cards in the spec is labeled as max width and not width in general. My interpretation is that for small width screens, width should be adjusted so that two cards always fit cleanly in each row (even when that means decreasing card size):

Should cards be flush with the right side of the container, instead of moving to a newline whenever they don't fit? It's a little frustrating since all designs have "perfect" screen sizes, but I think it may make more sense to use space-between instead of forcing gap to be 32px as labeled in the design.

Hmm good point, I suppose I can apply this to both the card and file-upload components and this would also eliminate the extra stories for large and compact. I see $_screen-size-MD is 768px, can we use this breaking point for small screen?
I see. This ties in with 1 and 3, if we can have set it to max-width that can dynamic adjust the card sizes while maintaining the gap and flushing to the right, that would be ideal.
Great question! I initially thought we could flush it to the right with auto-fit but yes it doesn't honor the gap specified in the design. We could bring this up with our designer.

Definitely will do 1, and play around to make 2 work, which should also solve 3

agliga

Couple of comments

First off, please target 18.3.0 branch
Second off, did you run npm run build? I see a lot of linting issues in the code. We did change that recently, so I want to make sure our linter is running correctly.

Looks good as a first pass overall.

agliga · 2024-09-04T15:18:25Z

src/modules/card.marko

@@ -0,0 +1,378 @@
+<div id="card">


This is good, but might be too generic. I think we do have an actual card module coming up.
We can call it file-upload-card maybe?
If anyone has any suggestions on this, feel free to add

agliga · 2024-09-04T15:19:42Z

src/sass/card/card.scss

+    height: inherit;
+    width: inherit;


Are these styles needed? They seem redundant.

Since I'm using the background-image it's needed since it doesn't take up any space. I could have used an img tag but I was thinking we need to overlay it with other elements, i.e. the progress bar so didn't want to take up block space

agliga · 2024-09-04T15:20:10Z

src/sass/card/card.scss

+@media screen and (max-width: $_screen-size-MD) {
+    .card {
+        width: $card-size-large;
+
+        .card__container {
+            height: $card-size-large;
+        }
+    }
+}


We usually put our media queries at the end of the file, so they're easy to be found.

agliga · 2024-09-04T15:21:41Z

src/sass/card/card.scss

+.card__label.icon-btn {
+    @include bubble-close();
+
+    position: absolute;


Why do we need position absolute. We should be able to put these inline I assume and style it that way.

Hmm I thought that was a simpler approach. I can use css grid and define quadrants where we can place these elements. Wdyt?

agliga · 2024-09-04T15:21:51Z

src/sass/card/card.scss

+.card .card__action {
+    @include bubble-close();
+
+    position: absolute;


Why do we need position absolute. We should be able to put these inline I assume and style it that way.

agliga · 2024-09-04T15:22:50Z

src/sass/file-upload/stories/file-upload-cards.stories.js

+    uploadedDocumentWithTitle as uploadedDocumentWithTitleCard,
+    uploadedDocumentWithTitleAndDescription as uploadedDocumentWithTitleAndDescriptionCard,
+    uploadedWithOverflow as uploadedWithOverflowCard,
+} from "../../card/stories/card.stories";


Interesting.
This is possibly a good pattern we can use in the future!

I noticed a lot of repetition so reused the markup from other stories. I tried this with the app but it seems like it there may be some special logic with the highlight-code component that doesn't process the functions

agliga · 2024-09-04T15:25:34Z

src/sass/variables/_variables.scss

+$card-size-default: 132px;
+$card-size-large: 168px;


Not sure if this is the right place to put these.
Taking a look at lightbox dialog, we have certain sizes defined in the :root of the file
https://github.com/eBay/skin/blob/18.3.0/src/sass/lightbox-dialog/lightbox-dialog.scss#L5-L8
I would follow that pattern

I wanted to put it in a place where both the card component and the file-upload component could use the var. Is there another shared location?

Removed these since the sizes will be dynamic and dictated by the parent

src/tokens/evo-dark.css

agliga · 2024-09-04T15:30:23Z

dist/card/card.css

+    background: none;
+    background: var(--color-background-on-secondary, #fff);


Two backgrounds here

agliga · 2024-09-04T15:31:12Z

src/sass/card/card.scss

+    top: var(--spacing-100, 8px);
+    right: var(--spacing-100, 8px);
+    margin: 0;
+    background: var(--color-background-on-secondary, #fff);


Use the mixin for the background please.
Make sure the first variable is not defined and relative to the component.
https://github.com/eBay/skin/blob/18.3.0/src/sass/lightbox-dialog/lightbox-dialog.scss#L177
So maybe @include background-color-token(card-action-color, color-background-on-secondary);

I'm not following, why would we provide a variable that is not defined in the first arg for the mixin?

ArtBlue

This is my first pass concentrating only on card and other general structural stuff. We can move on to the other sections of this PR when we've ironed out stuff for this.

Looks mostly good, but some important things that need to be addressed.

ArtBlue · 2024-09-04T13:57:21Z

dist/variables/_variables.scss

@@ -62,3 +62,5 @@ $font-size-large-1: $font-size-20;
 $font-size-medium: $font-size-16;
 $font-size-regular: $font-size-14;
 $font-size-small: $font-size-12;
+$card-size-default: 132px;


These are probably too specific to be in a global vars file. Since there aren't too many of them, I don't think a separate mixin is necessary. Are these necessary? Ultimately, if these modules are completely independent of one another (ideal), there shouldn't be any intertwined parameter dependencies across one another.

Ah I see, I had to use the size for the width in the css grid-template-cols style. Since we are changing the grid to allow it to display number of cols based on breakpoint, then this may not be needed anymore

ArtBlue · 2024-09-04T14:02:39Z

src/modules/card.marko

+            <div class="card">
+                <div class="card__container">
+                    <div class="card__content">
+                        <progress class="progress-bar" value=60 max=100/>


You need double quotes for attributes.

ArtBlue · 2024-09-04T14:02:45Z

src/modules/card.marko

+        <div class="card">
+            <div class="card__container">
+                <div class="card__content">
+                    <progress class="progress-bar" value=60 max=100/>


You need double quotes for attributes.

ArtBlue · 2024-09-04T14:07:47Z

src/modules/card.marko

I think the naming of this module may be a bit too generic. Since images are a staple of this module, maybe we use image-card? We can brainstorm offline.

ArtBlue · 2024-09-04T14:10:57Z

src/modules/card.marko

+    </div>
+
+    <highlight-code type="html">
+        <div class="card">


Do we need 3 layers here before we get to the content?

The other layer .card is for the card container which displays the thumbnail and the details underneath if provided. The .card-container is for the thumbnail and any overlay elements i.e. menu, label. Then the card content itself

I refactored it to remove the middle .container layer

ArtBlue · 2024-09-04T14:43:48Z

src/sass/card/card.scss

+@import "../variables/variables";
+
+.card {
+    display: flex;


Do we need flex here? These are standalone cards. I'm not seeing any complex structure inside.

ArtBlue · 2024-09-04T14:44:46Z

src/sass/card/card.scss

+}
+
+.card__container {
+    position: relative;


Why do we need relative positioning here?

Andrew had posted a similar comment earlier
#2435 (comment)

ArtBlue · 2024-09-04T14:48:28Z

src/sass/card/card.scss

+
+.card__container {
+    position: relative;
+    height: $card-size-default;


Why the height here? I think having a specified height here may be an issue for responsively reducing/enlarging these image cards. They will lose their aspect ratio. There's also the width above this layer. Not sure how they work together, but feels odd to have them in different layers.

Because the card can contain details, that's why the width and height are separate so that the width can be applied to the details as well since the card is the parent of the card container and the card details. Sounds like the size should be adjustable?

ArtBlue · 2024-09-04T14:55:27Z

src/sass/card/card.scss

+    height: $card-size-default;
+}
+
+@media screen and (max-width: $_screen-size-MD) {


We should be using the min-width approach instead. Have the default for the smallest screen and then enlarge as needed.

ArtBlue · 2024-09-04T16:01:44Z

src/sass/card/card.scss

It feels like we need another top-level module - image-card-group - for groups of these image-card items that would handle responsive display rules/views similar to how toggle-button and toggle-button-group work in tandem.

kentokage · 2024-09-04T17:10:05Z

Couple of comments

First off, please target 18.3.0 branch Second off, did you run npm run build? I see a lot of linting issues in the code. We did change that recently, so I want to make sure our linter is running correctly.

Looks good as a first pass overall.

Whoops, changed it to 18.3.0

Yep I ran npm run build and committed the the output from /dist

kentokage · 2024-09-06T19:52:55Z

Pending work

file-preview-card-group
add new modules to app
list-card is still there, but will ignore / remove since there is a future component similar to this down the line
ignore /dist and /src/modules/ for now

kentokage · 2024-09-18T21:55:22Z

Removed the dashed border for the responsive demo

fix: fix

ArtBlue

Overall, it's looking great!!! A few minor things I found...

ArtBlue · 2024-09-19T17:20:50Z

src/sass/file-preview-card-group/file-preview-card-group.scss

+    --row-preview-cards-min: 2;
+    --row-preview-cards-sm: 4;
+    --row-preview-cards-md: 5;
+    --row-preview-cards-lg: 7;
+    --row-preview-cards-xl: 9;
+    --row-preview-cards-xl2: 10;
+    --row-preview-cards-xl3: 12;
+    --row-preview-cards-xl4: 14;


Based on a recent decision to start using sass vars for private variables, we should convert these.

ArtBlue · 2024-09-19T17:22:57Z

src/sass/file-preview-card-group/file-preview-card-group.scss

+@supports not (contain: inline-size) {
+    @media (min-width: $_screen-size-SM) {
+        div.file-preview-card-group ul {
+            gap: var(--spacing-100, 8px);


Let's use the spacing vars from evo-core throughout instead of raw pixels for gaps.

I am using it, but I'll remove the fallback value

ArtBlue · 2024-09-19T17:25:38Z

src/sass/file-preview-card-group/stories/file-preview-card-group.stories.js

+export const base = () => `
+    <div class="file-preview-card-group">
+        <ul>
+            <li>${squareImage()}</li>


This is beautiful!!!

ArtBlue · 2024-09-19T17:28:13Z

src/modules/file-preview-card-group.marko

+    <h3>Base</h3>
+    <div class="demo">
+        <div class="demo__inner">
+            <div class="file-preview-card-group">


Weren't we going to do the responsive demos with an overflow container?

ArtBlue · 2024-09-19T18:36:12Z

src/sass/file-upload-input/file-upload-input.scss

+    border: 1px dashed;
+    display: flex;
+    flex-direction: row;
+    gap: var(--spacing-100, 8px);


I would avoid these backups. We'd want this to fail fast so we can correct the missing css prop.

ArtBlue · 2024-09-19T18:57:21Z

src/sass/file-preview-card/file-preview-card.scss

+
+    font-size: var(--font-size-14);
+    font-weight: 700;
+    height: 40px;


Use spacing css props.

ArtBlue · 2024-09-19T19:07:18Z

src/sass/file-preview-card/stories/file-preview-card.stories.js

+                        </svg>
+                    </span>
+                    <span class="inline-notice__main">
+                        <p>Upload failed</p>


Doesn't this individual card need something to indicate an error for screen readers?

This is removed

ArtBlue · 2024-09-19T19:12:46Z

src/sass/file-preview-card/stories/file-preview-card.stories.js

+    </div>
+`;
+
+export const portraitDoc = () => `


These PDF examples include an image. Let's make them consistent with how it's coded in Skin - for now these should be "other" with the generic icon.

I'll remove these PDF examples since the general one should cover it

ArtBlue · 2024-09-19T19:12:52Z

src/sass/file-preview-card/stories/file-preview-card.stories.js

+    </div>
+`;
+
+export const landscapeDoc = () => `


These PDF examples include an image. Let's make them consistent with how it's coded in Skin - for now these should be "other" with the generic icon.

I'll remove these PDF examples since the general one should cover it

ArtBlue · 2024-09-19T19:17:23Z

src/sass/file-preview-card-group/stories/file-preview-card-group.stories.js

+    </div>
+`;
+
+const portraitDocWithTitle = () => `


Please change all the PDF stories to match Skin as "other" than image - generic icon.

saiponnada

Minor doc changes, we also need to iron out aria-labels across the docs

src/sass/file-preview-card-group/stories/file-preview-card-group.stories.js

src/modules/file-preview-card.marko

src/sass/file-preview-card/stories/file-preview-card.stories.js

src/modules/file-preview-card.marko

agliga

Approving for now. I have one comment but its a docs issue. To me, this looks great and Im happy with how it turned out.
Docs change, we can always clean up after the fact.

agliga · 2024-09-20T19:43:17Z

src/modules/file-preview-card-group.marko

+        <div class="file-preview-card-group">
+            <ul>
+                <li>
+                    <!--refer to file-preview-card-->


Generally we like to include the full code, especially how its rendered above.
It might be more verbose but its much better developer experience.
That said, Im fine this being fixed later because its a docs change only.

One of the ideas suggested was to solve the verbose issue as you already mentioned and also not needing to constantly update the code as changes are being made to the skin css. Yep I can fix it later in a follow up PR

src/modules/file-preview-card.marko

ianmcburnie · 2024-09-24T19:48:18Z

So one last question before we merge this: should we rename from file-upload-input to just file-input?

dist/file-preview-card/file-preview-card.css

src/modules/file-preview-card.marko

kentokage · 2024-09-25T22:47:51Z

So one last question before we merge this: should we rename from file-upload-input to just file-input?

@ianmcburnie My intention with file-upload-input is that this is the selector and there would be a wrapper component on the ebayui-core side (maybe file-upload that would also incorporate all of the components (selector + preview card group). So these components would be part of the same family of components. But I can see file-input makes sense because there is no upload part yet. I'm fine with either way

src/sass/file-preview-card/file-preview-card.scss

src/modules/file-input.marko

ArtBlue

I took one last look. Looks good. Just a couple of last things.

ArtBlue · 2024-09-27T19:05:46Z

src/sass/file-preview-card-group/file-preview-card-group.scss

+//    - pixel values
+
+// Minimal Layout (default)
+$_row-preview-cards-min: 2;


SASS vars in modules are inherently private to the module. Underlines are not necessary. Let's please just remove them ti simplify.

src/sass/file-preview-card/stories/file-preview-card.stories.js

ianmcburnie

Very cool! Great work @kentokage ! Much appreciated.

ArtBlue

LGTM! Nice work!

kentokage added 2 commits September 3, 2024 10:18

feat(file-upload): new module

38e2ea2

feat(file-upload): more changes

8dc948d

fix(file-upload): update css feat(file-upload): fix css feat(file-upload): add modules feat(file-upload): fix module grammar

kentokage added 4 commits September 3, 2024 12:33

feat(file-upload): fixes

47b946d

feat(file-upload): add RTL stories

5aca7ed

feat(file-upload): add build

0c6a41c

feat(file-upload): add changeset

6ea051d

kentokage requested review from ArtBlue, agliga, ianmcburnie and LuLaValva September 3, 2024 21:40

kentokage marked this pull request as ready for review September 3, 2024 21:41

LuLaValva reviewed Sep 3, 2024

View reviewed changes

src/sass/card/card.scss Outdated Show resolved Hide resolved

src/sass/card/card.scss Outdated Show resolved Hide resolved

src/sass/card/stories/card-large.stories.js Outdated Show resolved Hide resolved

src/sass/file-upload/file-upload.scss Outdated Show resolved Hide resolved

kentokage added 2 commits September 3, 2024 17:35

feat(file-upload): feedback 1

cea0d1f

feat(file-upload): feedback 2

b1d620a

kentokage added 2 commits September 3, 2024 22:09

feat(file-upload): refactor to use media query

5182a53

feat(file-upload): remove unnecessary css

3b378a3

agliga reviewed Sep 4, 2024

View reviewed changes

ArtBlue requested changes Sep 4, 2024

View reviewed changes

kentokage changed the base branch from master to 18.3.0 September 4, 2024 17:08

kentokage added 3 commits September 5, 2024 11:48

feat(file-upload-input): refactor components

634eab4

feat(file-upload): refactor file-preview-card

fe1b3fb

feat(file-upload): address comments

63bf028

kentokage added 4 commits September 8, 2024 00:35

feat(file-upload): add card group

7683ad6

feat(file-upload): update story

ebfdf46

feat(file-upload): refactor card

837c521

feat(file-upload): refactor cleaner

796892d

feat(file-upload): remove upload failed state and fix style

5823a86

fix: fix

ArtBlue requested changes Sep 19, 2024

View reviewed changes

saiponnada reviewed Sep 19, 2024

View reviewed changes

src/sass/file-preview-card-group/stories/file-preview-card-group.stories.js Outdated Show resolved Hide resolved

src/modules/file-preview-card.marko Show resolved Hide resolved

src/sass/file-preview-card/stories/file-preview-card.stories.js Outdated Show resolved Hide resolved

ianmcburnie reviewed Sep 19, 2024

View reviewed changes