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.

Sign up for GitHub

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

fix(homepage): a11y errors #1483

Merged
merged 17 commits into from
Mar 8, 2022
Merged

fix(homepage): a11y errors #1483

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
9a73792
fix (homepage): a11y empty buttons
0618 Mar 3, 2022
d6eeaf1
fix(docs homepage): a11y for install copy
0618 Mar 4, 2022
73dc138
fix(homepage): a11y issue of the empty label from install code
0618 Mar 4, 2022
e9a2636
fix (homepage): a11y for rating icons
0618 Mar 4, 2022
1d2b644
fix some type error
0618 Mar 4, 2022
2ee9a60
fix(homepage): a11y ARIA reference
0618 Mar 4, 2022
212de8a
fix(homepage): a11y langguage missing
0618 Mar 4, 2022
365f6eb
fix(homepage): a11y fieldset description
0618 Mar 4, 2022
2c07563
update changeset
0618 Mar 5, 2022
b1222e5
Update docs/src/pages/HomePrimitivePreview.tsx
0618 Mar 8, 2022
1a16af5
resolve review comments
0618 Mar 8, 2022
ca834d4
Merge branch 'fix-a11y-home' of github.com:aws-amplify/amplify-ui int…
0618 Mar 8, 2022
94bb127
fix TextField a11y error when lablelHidden
0618 Mar 8, 2022
b948437
add ariaLabel to MenuButton
0618 Mar 8, 2022
20f84aa
refactor: use ComponentClassNames.VisuallyHidden var to replace ampli…
0618 Mar 8, 2022
dde5608
convert primitive related changes. Will do in another PR
0618 Mar 8, 2022
7a40db3
remove unused import
0618 Mar 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .changeset/quiet-impalas-chew.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
'@aws-amplify/ui-react': patch
Copy link
Contributor

Choose a reason for hiding this comment

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

with the new attributes being added and the structure of some of the components being updated do we want this to be a minor bump instead of a patch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point...

I checked the semver doc

Minor version Y (x.Y.z | x > 0) MUST be incremented if new, backwards compatible functionality is introduced to the public API. It MUST be incremented if any public API functionality is marked as deprecated. It MAY be incremented if substantial new functionality or improvements are introduced within the private code. It MAY include patch level changes. Patch version MUST be reset to 0 when minor version is incremented.

Since we don't have any new features in this PR, I think it's not a MUST.

'@aws-amplify/ui': patch
---

- fix a11y erros on docs homepage
- fix type errors
- rename css class
9 changes: 7 additions & 2 deletions docs/src/components/Copy.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { CopyToClipboard } from 'react-copy-to-clipboard';

import { Button } from '@aws-amplify/ui-react';

export const Copy = ({ text, size, variation }) => {
export const Copy = ({ text, size, variation, className }) => {
const [copied, setCopied] = React.useState(false);

const copy = () => {
Expand All @@ -15,7 +15,12 @@ export const Copy = ({ text, size, variation }) => {

return (
<CopyToClipboard text={text} onCopy={copy}>
<Button size={size} variation={variation} isLoading={copied}>
<Button
size={size}
variation={variation}
isLoading={copied}
className={className}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add className because of the modification on docs/src/pages/index.page.tsx

>
{copied ? 'Copied!' : 'Copy'}
</Button>
</CopyToClipboard>
Expand Down
1 change: 1 addition & 0 deletions docs/src/components/Layout/Header.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ export const Header = ({ platform, colorMode, setColorMode }) => {
<Button
className="docs-header-menu-button"
onClick={() => setExpanded(!expanded)}
aria-label="Docs header menu button"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we may eventually use variables for aria-label for i18n.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please use ariaLabel instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

>
{expanded ? <IconClose /> : <IconMenu />}
</Button>
Expand Down
8 changes: 4 additions & 4 deletions docs/src/pages/HomePrimitivePreview.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,16 +57,16 @@ export const HomePrimitivePreview = () => {
isExclusive
onChange={(value: string) => setExclusiveValue(value)}
>
<ToggleButton value="align-left">
<ToggleButton value="align-left" aria-label="align-left">
<IconFormatAlignLeft />
</ToggleButton>
<ToggleButton value="align-center">
<ToggleButton value="align-center" aria-label="align-center">
<IconFormatAlignCenter />
</ToggleButton>
<ToggleButton value="align-right">
<ToggleButton value="align-right" aria-label="align-right">
<IconFormatAlignRight />
</ToggleButton>
<ToggleButton value="align-justify">
<ToggleButton value="align-justify" aria-label="align-justify">
<IconFormatAlignJustify />
</ToggleButton>
</ToggleButtonGroup>
Expand Down
2 changes: 1 addition & 1 deletion docs/src/pages/_document.page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ const getCSPContent = (context: Readonly<HtmlProps>) => {
class MyDocument extends Document {
render() {
return (
<Html>
<Html lang="en-us">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we may eventually use variables for lang

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you create a ticket for the docs for a language switcher if it doesn't already exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ticket cut: https://app.asana.com/0/1201840995372760/1201937080448144/f

<Head>
<meta
httpEquiv="Content-Security-Policy"
Expand Down
26 changes: 14 additions & 12 deletions docs/src/pages/index.page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import { HomePrimitivePreview } from './HomePrimitivePreview';
import { Copy } from '@/components/Copy';
import { Footer } from '@/components/Layout/Footer';

import type { SandpackThemeProp } from '@codesandbox/sandpack-react';
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, we are going to remove sandpack in favor of react-live. #1395 @dbanksdesign

Copy link
Contributor

Choose a reason for hiding this comment

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

potentially... there are some potential security issues with it since it uses eval, still up for debate


const code = `import { AmplifyProvider, Button, Card, Text, Heading, Flex, Badge, Image, StepperField, useTheme } from '@aws-amplify/ui-react';
import '@aws-amplify/ui-react/styles.css';

Expand Down Expand Up @@ -100,7 +102,7 @@ const HomePage = ({ colorMode, setThemeOverride, themeOverride }) => {
const router = useRouter();
const { tokens } = useTheme();
const framework = (router.query.platform as string) ?? 'react';
const sandPackTheme = {
const sandPackTheme: SandpackThemeProp = {
palette: {
activeText: `${tokens.colors.font.interactive}`,
defaultText: `${tokens.colors.font.secondary}`,
Expand Down Expand Up @@ -133,7 +135,6 @@ const HomePage = ({ colorMode, setThemeOverride, themeOverride }) => {
monoFont:
'"Fira Mono", "DejaVu Sans Mono", Menlo, Consolas, "Liberation Mono", Monaco, "Lucida Console", monospace',
fontSize: '16px',
lineHeight: '1.5',
},
};
const installScripts = {
Expand Down Expand Up @@ -170,16 +171,17 @@ const HomePage = ({ colorMode, setThemeOverride, themeOverride }) => {
Get started
<IconChevronRight />
</Button>
<TextField
label=""
labelHidden={true}
isReadOnly={true}
className="install-code"
outerEndComponent={
<Copy variation="link" text={frameworkInstallScript} />
}
value={frameworkInstallScript}
/>
<code className="install-code__container">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

<code> tag might be more proper here

<p className="install-code__content">
{frameworkInstallScript}
</p>
<Copy
className="install-code__button"
size=""
variation="link"
text={frameworkInstallScript}
/>
</code>
</Flex>
</Card>
<Flex
Expand Down
14 changes: 12 additions & 2 deletions docs/src/styles/docs/home.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,19 @@
padding: var(--amplify-space-xxl);
}

.install-code {
.install-code__container {
flex-grow: 1;
margin: 0;
display: flex;
}

.install-code__content {
margin: auto;
overflow-x: auto;
}

.install-code__button {
place-self: end;
align-self: center;
}

.docs-home-amp-product-card {
Expand Down
2 changes: 2 additions & 0 deletions packages/react/src/components/Authenticator/SignIn/SignIn.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ export function SignIn() {
className="amplify-flex"
disabled={isPending}
>
<legend>Sign in</legend>
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this be translated? Also, this changes the design of the Authenticator. Should this be visually hidden @ErikCH @wlee221

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes this will change the design of Authenticator. It would be better if you can ping them to take a look at

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this fix an accessibility issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there's an alert that fieldset missing legend

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot to add a amplify-visually-hidden class...I'll do that

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, adding visually-hidden should be sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Added amplify-visually-hidden

<UserNameAlias
labelHidden={userOverrides?.labelHidden}
placeholder={userOverrides?.placeholder}
Expand Down Expand Up @@ -100,6 +101,7 @@ export function SignIn() {

SignIn.Header = (): JSX.Element => null;
SignIn.Footer = () => {
// eslint-disable-next-line react-hooks/rules-of-hooks
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not disable rules of hooks here. There are a handful of rules of hooks violations in Authenticator which need to be dealt with and I don't want to have to find them manually.

const { toResetPassword } = useAuthenticator();

// Support backwards compatibility for legacy key with trailing space
Expand Down
2 changes: 1 addition & 1 deletion packages/react/src/primitives/Menu/Menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ const MenuPrimitive: Primitive<MenuProps, 'div'> = (
<DropdownMenuTrigger asChild={true}>
{trigger ?? (
<MenuButton
role="button"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure why we need to add it for a button, so removed it

Copy link
Contributor

Choose a reason for hiding this comment

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

Did it cause an accessibility error? I vaguely remember that without role="button" I couldn't use my own button with the Radix UI Dropdown Menu. Can you make sure removing this didn't break the Menu?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, it doesn't cause any a11y error because it is a button. It would cause error if it's a <div> or something but uses as a button

Copy link
Contributor

Choose a reason for hiding this comment

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

Tested it and the menu button still works correctly. Proceed 👍

ariaLabel="menu button"
Copy link
Contributor

@reesscot reesscot Mar 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we want to add a standard ariaLabel here. Remember, this will apply for all customers using the Menu. We may need to consider adding an external prop to set this label. To solve the "missing label" problem, you could pass in the MenuButton component as the trigger prop in the docs page that uses this Menu component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Done

size={size}
testId={MENU_TRIGGER_TEST_ID}
className={classNames(
Expand Down
8 changes: 3 additions & 5 deletions packages/react/src/primitives/Rating/RatingIcon.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,9 @@ export const RatingIcon: React.FC<RatingIconProps> = ({
className,
}) => {
return (
<View as="span" className={classNames(`amplify-rating-icon-container`)}>
<View as="label" className={classNames(`amplify-rating-label`)}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure what's the label for, so removed it

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I'm not sure why there's a label here either. @jacoblogan Do you remember why we used a label here? We should take another look at the accessibility of this component. If we test this in VoiceOver does it read out the visually hidden text? Is there potentially a better way to label a rating component that we haven't seen yet?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the label tag was a remnant of earlier designs when this component was not read only. Looking at some other rating components out there they use a label and hidden checkbox to mark user selections. @reesscot brought up a good point that we could have customers who are targeting this label for styling and so if we don't need to remove it, then it would be best to leave it in. @0618 other than the extra clutter, was the label causing any problems? If not then I agree that we should leave it in as to not create a potentially breaking change for customers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jacoblogan yes, "empty label" is a type of a11y error, so I removed it but kept the class selector in this PR. Is that ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. If it's causing a11y error let's remove it.

<View as="span" className={classNames(className)} color={fill}>
{icon}
</View>
<View as="span" className="amplify-rating-icon-container">
<View as="span" className={classNames(className)} color={fill}>
{icon}
</View>
</View>
);
Expand Down
16 changes: 8 additions & 8 deletions packages/react/src/primitives/Rating/RatingMixedIcon.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,29 +19,29 @@ export const RatingMixedIcon: React.FC<RatingMixedIconProps> = ({
}) => {
const widthPercentage = `${(value % 1) * 100}%`;
return (
<View as="span" className={classNames(`amplify-rating-icon-container`)}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure why we need classNames here, so removed it

<View as="label" className={classNames(`amplify-rating-label`)}>
<View as="span" className="amplify-rating-icon-container">
<View as="span" className="amplify-rating-icon-place">
<View
as="span"
className={classNames(
`amplify-rating-icon`,
`amplify-rating-icon-empty`
'amplify-rating-icon',
'amplify-rating-icon-empty'
)}
color={emptyColor}
>
{emptyIcon}
</View>
</View>
<View
as="label"
className={classNames(`amplify-rating-label`)}
as="span"
className="amplify-rating-icon-place"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't change this classname

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

width={widthPercentage}
>
<View
as="span"
className={classNames(
`amplify-rating-icon`,
`amplify-rating-icon-filled`
'amplify-rating-icon',
'amplify-rating-icon-filled'
)}
color={fillColor}
>
Expand Down
3 changes: 2 additions & 1 deletion packages/react/src/primitives/SliderField/SliderField.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ const SliderFieldPrimitive: Primitive<SliderFieldProps, typeof Root> = (
const fieldId = useStableId(id);
const labelId = useStableId();
const descriptionId = useStableId();
const ariaDescribedBy = labelHidden ? labelId : descriptionId;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const ariaDescribedBy = labelHidden ? labelId : descriptionId;
const ariaDescribedBy = descriptiveText ? descriptionId : undefined; // only set `ariaDescribedBy` if descriptiveText is provided

Copy link
Contributor

Choose a reason for hiding this comment

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

Please also add a unit test for this.

Copy link
Contributor Author

@0618 0618 Mar 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Weird...I removed the code in this commit 94bb127 but it still shows up in the latest PR...

NVM, I missed SliderField.tsx, only changed TextField.tsx

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, can we please split these changes out into a separate PR, as we will need to make these changes in all the Field components that use the FieldDescription component.


const { flexContainerStyleProps, rest } = splitPrimitiveProps(_rest);

Expand Down Expand Up @@ -133,7 +134,7 @@ const SliderFieldPrimitive: Primitive<SliderFieldProps, typeof Root> = (
/>
</Track>
<Thumb
aria-describedby={descriptionId}
aria-describedby={ariaDescribedBy}
aria-labelledby={labelId}
aria-valuetext={ariaValuetext}
className={ComponentClassNames.SliderFieldThumb}
Expand Down
8 changes: 5 additions & 3 deletions packages/react/src/primitives/TextField/TextField.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ const TextFieldPrimitive = <Multiline extends boolean>(

const fieldId = useStableId(id);
const descriptionId = useStableId();
const labelId = useStableId();
const ariaDescribedBy = labelHidden ? labelId : descriptionId;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing this is indented to fix an accessibility error where the description text is missing when labelHidden is true. I don't think it makes sense for a field to have the same text for label and description since it would be redundant for screen reader customers (might be read twice).

I'm seeing two accessibility issues here:

  1. Current we hide the entire description if labelHidden is true (meaning that we hide valuable accessibility descriptions app developers could provide for users, even if it's visually hidden). We might as well still render the description, but hide it if labelHidden is true.
  2. When app devs don't provide a description (which is most of the time), we still set ariaDescribedBy to an id that doesn't match an element in the DOM. I think this is the "5 X Broken ARIA reference" you are referring to, right?

I think the behavior should actually be changed to something like the following to fix these issues:

  1. Update FieldDescription component to not return null when the labelHidden is true (it should still have a description, just be wrapped in VisuallyHidden component). We'll need to verify this doesn't break any styling, it may affect the field when flex direction is row (thinking it may leave extra room).
  2. Change above logic to set ariaDescribedBy to undefined when description is empty (there's no reason to have an ariaDescribedBy prop id set if there's no description text. And there is usually no description text.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree @reesscot's fixes

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, agree! Thanks for the findings here and fixing this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated! Converted the change and hide FieldDescription if labelHidden is true, instead of FieldDescription returning null.


const { flexContainerStyleProps, baseStyleProps, rest } =
splitPrimitiveProps(_rest);
Expand All @@ -61,7 +63,7 @@ const TextFieldPrimitive = <Multiline extends boolean>(
const { rows } = props;
control = (
<TextArea
aria-describedby={descriptionId}
aria-describedby={ariaDescribedBy}
hasError={hasError}
id={fieldId}
ref={isTextAreaRef(props, ref) ? ref : undefined}
Expand All @@ -75,7 +77,7 @@ const TextFieldPrimitive = <Multiline extends boolean>(
const { type = 'text' } = props;
control = (
<Input
aria-describedby={descriptionId}
aria-describedby={ariaDescribedBy}
hasError={hasError}
id={fieldId}
ref={isInputRef(props, ref) ? ref : undefined}
Expand All @@ -98,7 +100,7 @@ const TextFieldPrimitive = <Multiline extends boolean>(
testId={testId}
{...flexContainerStyleProps}
>
<Label htmlFor={fieldId} visuallyHidden={labelHidden}>
<Label htmlFor={fieldId} visuallyHidden={labelHidden} id={labelId}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this? the htmlFor is handling the link to the input field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

{label}
</Label>
<FieldDescription
Expand Down
5 changes: 5 additions & 0 deletions packages/react/src/primitives/types/button.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,9 @@ export interface ButtonProps extends ViewProps, FlexContainerStyleProps {
* Changes the visual weight of the button.
*/
variation?: ButtonVariations;

/**
* Changes href of the button.
*/
href?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about this change. Most buttons are not a tags, so it seems misleading to offer an href prop for them. Where are we using a button with an href?

Copy link
Contributor

Choose a reason for hiding this comment

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

We are using it in some places on the docs site where we do <Button as={Link} or <Button as="a". I agree it is a bit weird to have href on the button type. I wonder if there is a better way to get a link that looks like a button?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1.. Should we just use an a tag in the case?

Copy link
Contributor Author

@0618 0618 Mar 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Good point! I think we should not use buttons as anchor or vice versa. The best way is to make button-link and link-button CSS styles.

I'll remove the type change here and add a ticket for refactor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Revert change.

Cut a ticket to use Link to refactor the "button-links"

}
9 changes: 5 additions & 4 deletions packages/ui/src/theme/css/component/card.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,18 @@
box-shadow: var(--amplify-components-card-box-shadow);
display: inline-block;
Copy link
Contributor

Choose a reason for hiding this comment

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

Anyone know why cards are inline-block? Seems like they should just be display: block, then you wouldn't need the width:100%. @dbanksdesign thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Changing this is a breaking change as well I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Changed to block

padding: var(--amplify-components-card-padding);

&[data-variation="outlined"] {
width: 100%;

&[data-variation='outlined'] {
background-color: var(--amplify-components-card-outlined-background-color);
border-radius: var(--amplify-components-card-outlined-border-radius);
border-width: var(--amplify-components-card-outlined-border-width);
border-style: var(--amplify-components-card-outlined-border-style);
border-color: var(--amplify-components-card-outlined-border-color);
box-shadow: var(--amplify-components-card-outlined-box-shadow);
}
&[data-variation="elevated"] {

&[data-variation='elevated'] {
background-color: var(--amplify-components-card-elevated-background-color);
border-radius: var(--amplify-components-card-elevated-border-radius);
border-width: var(--amplify-components-card-elevated-border-width);
Expand Down
2 changes: 1 addition & 1 deletion packages/ui/src/theme/css/component/rating.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
width: 1em;
}

.amplify-rating-label {
.amplify-rating-icon-place {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

since the label tag is changed to span, rename the class

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we can rename classnames without a breaking change, since customers could be depending on them. These files affect customer apps and not just the docs.

Copy link
Contributor

@zchenwei zchenwei Mar 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, making changes to class name are breaking changes. I believe my demo app has use some of the classnames for custom styling

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted back.

position: absolute;
overflow: hidden;
height: 1em;
Expand Down