Skip to content

Conversation

@ross-pfahler
Copy link
Collaborator

Closes RSP-1569

✅ Pull Request Checklist:

  • Included link to corresponding Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Team:

@adobe-bot
Copy link

Build successful! 🎉

@codecov
Copy link

codecov bot commented Mar 1, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@2c465f8). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #239   +/-   ##
=========================================
  Coverage          ?   70.57%           
=========================================
  Files             ?      206           
  Lines             ?     3973           
  Branches          ?      847           
=========================================
  Hits              ?     2804           
  Misses            ?      587           
  Partials          ?      582
Impacted Files Coverage Δ
...es/@react-spectrum/progress/src/ProgressCircle.tsx 80.95% <ø> (ø)

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

</div>
```

### Indeterminate
Copy link
Member

Choose a reason for hiding this comment

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

Should we include this as part of the value section (similar to Checkbox)? @dannify

Copy link
Member

Choose a reason for hiding this comment

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

Yeah it is an important option that needs a little explanation. Let's move it to be under value like Checkbox

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

LFDanLu
LFDanLu previously approved these changes Mar 3, 2020
Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

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

LGTM

@adobe-bot
Copy link

Build successful! 🎉

dannify
dannify previously approved these changes Mar 4, 2020
@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

Copy link
Member

@ktabors ktabors left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@ktabors ktabors left a comment

Choose a reason for hiding this comment

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


## Value

By default Progress Circles are controlled with the `value` prop representing current percentage as an integer.
Copy link
Member

Choose a reason for hiding this comment

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

I noticed that we have Progress Circles as two words instead of one ProgressCircles throughout the docs.


### Accessibility

ProgressCircle requires an `aria-label` or `aria-labelledby` attribute.
Copy link
Member

Choose a reason for hiding this comment

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

This is very direct and clear, but I like Meter's way of saying this.
Depending on the visualization being used (i.e. depending on the showValueLabel prop), a label, aria-label, or aria-labelledby attribute are required.

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

ktabors
ktabors previously approved these changes Mar 12, 2020
Copy link
Member

@ktabors ktabors left a comment

Choose a reason for hiding this comment

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

lgtm

</div>
```

If using the over background variant, ensure the background offers enough contrast for the ProgressCircle to be legible and meets color contrast guidelines.
Copy link
Contributor

@MidnightCoder06 MidnightCoder06 Mar 12, 2020

Choose a reason for hiding this comment

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

is there a difference in your reference to the over background variant here vs. when you referenced it on line 88 but put it in backticks that time?
https://github.com/adobe-private/react-spectrum-v3/pull/239/files#diff-2900858ca68bc0d0ca2864bb6cfbc5bbR88

Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

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

Since we used to have 'centered' prop... do we want to tell people here that they should just center it themselves (we recommend flex)? or do we just not mention it because we shouldn't teach them how to do things like that because that should be a known thing?

@adobe-bot
Copy link

Build successful! 🎉

@snowystinger snowystinger mentioned this pull request Mar 13, 2020
5 tasks
@dannify
Copy link
Member

dannify commented Mar 13, 2020

@snowystinger Good idea for a "FAQ" on the page maybe?

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@dannify dannify merged commit 8108375 into master Mar 13, 2020
@dannify dannify deleted the progress-circle-docs branch March 13, 2020 23:44
fezproof pushed a commit to fezproof/projects that referenced this pull request Aug 19, 2020
* * updating TableView documentation at https://react-spectrum.corp.adobe.com/components/TableView.
* for issues:
  adobe/react-spectrum#239
  adobe/react-spectrum#240

* Update TableView.mdx

added explanation for needing styling

* Update TableView.mdx

* missing `%` in TableView.styl
* changed heading to "Why doesn't my table render" instead of "Example source code" and moved code to bottom

* - added style height to div in StoryBook for stories/TableView.js, so deleted stories/TableView.styl
- updated TableView.mdx with simpler troubleshooting and links to examples

* Update TableView.mdx

* Removing link that contains contradictory documentation

If we need a simpler example, we can explore that in another PR
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.

9 participants