feat: add Attachment component #220

levithomason · 2018-09-13T02:15:36Z

Attachment

An Attachment displays a file attachment.

TODO

API Proposal

This PR is implementing the Attachment component from stardust-ui/specifications#1. The latest Teams theme redlines were used. Sizes, spacing, colors, and states are redline correct for this version of the component.

<Attachment
  icon="file word outline"    // font awesome, for now
  header="Document.docx"  // Text shorthand
  description="800 Kb"     // Text shorthand
  action={{ icon: 'x' }}      // Button shorthand
  progress={33}               // percent complete
/>

The markup includes a div for each slot. This makes flexbox and css grid layouts easier.

<div class="ui-attachment">
  <!-- Icon -->
  <div class="...">
    <span class="ui-icon" aria-hidden="true"></span>
  </div>

  <!-- Header / Description -->
  <div class="...">
    <div class="...">Document.docx</div>
    <div class="...">800 Kb</div>
  </div>

  <!-- Action -->
  <div class="...">
    <button class="ui-button" aria-disabled="false">
        <span class="ui-icon" aria-hidden="true"></span>
    </button>
  </div>

  <!-- Progress -->
  <div class="..."></div>
</div>

Actionable

This prop styles the component to appear actionable by the user. A pointer cursor is used and a hover color is applied.

<Attachment actionable ... />

Things to know

Limited features: This PR only introduces the base component and its slots. Other PRs will handle the various types and colors of Attachments.
Icon: These are currently font awesome icons, pending feat(icons): add support for styling SVG icons #183 and subsequent work.
Action: The action (Button) likely needs more specific positioning and styling to match the Teams theme. This will require some button updates and maybe some theme updates. A separate PR should handle this.
Multiple Actions - The spec called for multiple actions, however, this usage was limited to Slack. Gmail, Teams, and Outlook all use a single action. A single action slot was used instead. The render prop callback can be used to provide a button group if the user wishes.

notandrew · 2018-09-14T21:46:01Z

src/themes/teams/components/Attachment/attachmentStyles.ts

+  }),
+
+  description: ({ props, variables }: StyleParam): ICSSInJSStyle => ({
+    opacity: 0.5,


Can you remove this? We don't want opacity set for the description...

notandrew · 2018-09-14T21:47:03Z

src/themes/teams/components/Attachment/attachmentStyles.ts

+  }),
+
+  progress: ({ props, variables }: StyleParam): ICSSInJSStyle => ({
+    transition: 'width 0.2s',


looks like the animation's working!

notandrew · 2018-09-14T21:57:27Z

src/themes/teams/components/Attachment/attachmentStyles.ts

+
+  icon: ({ props, variables }: StyleParam): ICSSInJSStyle => ({
+    flex: '0 0 auto',
+    marginRight: variables.iconSpace,


I think we don't want this margin since the attachment's 8px padding is enough to get this icon in the correct place.

This is between the icon and the header/description. I believe the redline called for a larger value here.

mnajdova · 2018-09-15T05:52:35Z

src/components/Attachment/Attachment.tsx

+              fitted: true,
+            },
+          })}
+        </div>


I am wondering if this is the best approach, adding div's for handling our styling. I don't see a way how can the user alter these, unless using CSS selectors. I had similar propose #199 (comment) but I don't think thins should be the general approach when defining shorthands (see the comments below). What are your thoughts on this @levithomason ?

@mnajdova, sorry, but just to be sure - does your comment above, essentially, attribute to the problem of customizing slot's tree (the one that referenced issue is about)?

Yes, exactly. Not strictly for this component, but just as e reference to the RFC about the customizing of the slot's tree.

kuzhelov · 2018-09-17T13:59:24Z

docs/src/examples/components/Attachment/Slots/index.tsx

+import ComponentExample from 'docs/src/components/ComponentDoc/ComponentExample'
+import ExampleSection from 'docs/src/components/ComponentDoc/ExampleSection'
+
+const Slots = () => (


one thing that am thinking about - probably, it would be nice it we would come up with some general logic of presenting these slots in a way that those will be more visible to the client

kuzhelov · 2018-09-17T14:14:44Z

docs/src/examples/components/Attachment/Slots/index.tsx

+      examplePath="components/Attachment/Slots/AttachmentIconExample"
+    />
+    <ComponentExample
+      title="Header"


header is advertised as slot, and maybe I've made a false conclusion from it, but the first thing I've tried was the following - essentially, one of those things that we might expect from the shorthands:

<Attachment header={{ content: '...', styles: { color: 'red' } }} />

Apparently, it doesn't work. So, the question is the following: do we see this general pattern (that is applicable for shorthands) to be applicable to any slot content? Actually, more interested about our vision on that.

To me it seems to be a very good thing to ensure this consistency in our customization mechanisms - especially given that the following approach for style tweaking seems to be deprecated:

<Attachment styles={{ header: {..<header styles>..} }}

@kuzhelov will this PR solve that? #238

@brianespinosa, sorry for the late response, my bad :( my promise to be better next time.

@kuzhelov will this PR solve that? #238

Unfortunately, now - and the reason is that header is currently introduced as a simple string, and not a Stardust component that encapsulates this knowledge on how this styling should be applied.

However, this is quite easy to fix - we could use Text component for header and content, so that all functionality of slots (where main aspect is styling) will be preserved.

kuzhelov · 2018-09-17T14:19:56Z

docs/src/examples/components/Attachment/Variations/index.tsx

+  <ExampleSection title="Variations">
+    <ComponentExample
+      title="Actionable"
+      description="An Attachment can be styled to indicate possible user interaction."


not sure, but maybe we could better advertise to the user that this (and couple of other ones) example is clickable - frankly, was able to infer this fact only by looking on the code :)

codecov · 2018-09-19T15:52:02Z

Codecov Report

Merging #220 into master will increase coverage by 0.16%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #220      +/-   ##
==========================================
+ Coverage   92.19%   92.36%   +0.16%     
==========================================
  Files          61       63       +2     
  Lines        1127     1152      +25     
  Branches      147      173      +26     
==========================================
+ Hits         1039     1064      +25     
  Misses         84       84              
  Partials        4        4

Impacted Files	Coverage Δ
src/components/Attachment/index.ts	`100% <100%> (ø)`
src/components/Attachment/Attachment.tsx	`100% <100%> (ø)`
src/index.ts	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09998f8...c91e5ce. Read the comment docs.

kuzhelov · 2018-09-19T22:41:53Z

lets address pending comments that were left for already provided functionality, and merge this PR afterwards.

After that there will be additional PR to solve the rest two requirements:

Handle focus style outline
add colors for various file types

Will take care of that.

mnajdova · 2018-09-20T14:45:59Z

src/components/Attachment/Attachment.tsx

+        {(header || description) && (
+          <div className={classes.content}>
+            {Text.create(header, {
+              defaultProps: {


levithomason requested review from dvdzkwsk, kuzhelov, miroslavstastny and mnajdova as code owners September 13, 2018 02:15

levithomason added the 🚧 WIP label Sep 13, 2018

levithomason force-pushed the feat/attachment branch 2 times, most recently from 06d915c to f92ef7c Compare September 14, 2018 04:08

levithomason changed the title ~~[WIP] feat: add Attachment component~~ feat: add Attachment component Sep 14, 2018

levithomason added 🚀 ready for review and removed 🚧 WIP labels Sep 14, 2018

levithomason force-pushed the feat/attachment branch 3 times, most recently from 8adbc0a to fab102f Compare September 14, 2018 04:38

levithomason mentioned this pull request Sep 14, 2018

Attachment component spec stardust-ui/specifications#1

Open

levithomason force-pushed the feat/attachment branch 2 times, most recently from a4cb6b6 to dcac475 Compare September 14, 2018 04:57

feat: add Attachment component

4e3dd5d

levithomason force-pushed the feat/attachment branch from dcac475 to 4e3dd5d Compare September 14, 2018 07:40

notandrew approved these changes Sep 14, 2018

View reviewed changes

notandrew reviewed Sep 14, 2018

View reviewed changes

mnajdova reviewed Sep 15, 2018

View reviewed changes

kuzhelov reviewed Sep 17, 2018

View reviewed changes

kuzhelov approved these changes Sep 17, 2018

View reviewed changes

kuzhelov added ready for merge and removed 🚀 ready for review labels Sep 17, 2018

make docs examples more intuitive

6e96dca

kuzhelov-ms added 9 commits September 20, 2018 11:55

fix types

d69b534

introduce shorthands for description and header

e884771

introduce tests for shorthand props

9ddd25e

remove not supported 'fitted' prop passed to Icon shorthand

0962ef7

introduce test for 'icon' shorthand prop

ba945d9

rearrange docs sections

b46834d

add tests for 'action' slot

a3f8940

make examples consistent: add alerting on 'close' icon click

ac16214

restrict max width of progress bar

3ed909c

mnajdova reviewed Sep 20, 2018

View reviewed changes

mnajdova approved these changes Sep 20, 2018

View reviewed changes

kuzhelov-ms and others added 8 commits September 24, 2018 17:28

Merge branch 'master' into feat/attachment

6fe8482

Merge branch 'master' into feat/attachment

9af65a9

update changelog

0f3f914

remove unused import

9c6e22f

remove duplicate declaration of 'create' method

a92bb96

fix style types

adeeb4d

fix lint issues

6566418

revert changes in Text

c91e5ce

kuzhelov merged commit 5d842a0 into master Sep 24, 2018

levithomason deleted the feat/attachment branch September 25, 2018 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add Attachment component #220

feat: add Attachment component #220

levithomason commented Sep 13, 2018 •

edited by kuzhelov

notandrew Sep 14, 2018

notandrew Sep 14, 2018

notandrew Sep 14, 2018

levithomason Sep 17, 2018

mnajdova Sep 15, 2018

kuzhelov Sep 17, 2018

mnajdova Sep 17, 2018

kuzhelov Sep 17, 2018

kuzhelov Sep 17, 2018 •

edited

brianespinosa Sep 17, 2018

kuzhelov Sep 19, 2018

kuzhelov Sep 17, 2018

codecov bot commented Sep 19, 2018 •

edited

kuzhelov commented Sep 19, 2018 •

edited

mnajdova Sep 20, 2018

feat: add Attachment component #220

feat: add Attachment component #220

Conversation

levithomason commented Sep 13, 2018 • edited by kuzhelov

Attachment

TODO

API Proposal

Actionable

Things to know

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuzhelov Sep 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 19, 2018 • edited

Codecov Report

kuzhelov commented Sep 19, 2018 • edited

Choose a reason for hiding this comment

levithomason commented Sep 13, 2018 •

edited by kuzhelov

kuzhelov Sep 17, 2018 •

edited

codecov bot commented Sep 19, 2018 •

edited

kuzhelov commented Sep 19, 2018 •

edited