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

Jump to bottom

Accessibility: replace padding values with logical properties #774

Merged
merged 5 commits into from Nov 24, 2023
Merged

Accessibility: replace padding values with logical properties #774

merged 5 commits into from Nov 24, 2023

Conversation

create-issue-branch[bot]
Copy link
Contributor

closes #773

@create-issue-branch create-issue-branch bot temporarily deployed to previews/a11y/773-Accessibility_replace_padding_values_with_logical_properties November 21, 2023 09:03 Inactive
@github-actions GitHub Actions
Copy link

@github-actions GitHub Actions
Copy link

@magdalenajadach magdalenajadach temporarily deployed to previews/a11y/773-Accessibility_replace_padding_values_with_logical_properties November 23, 2023 17:37 — with GitHub Actions Inactive
@magdalenajadach magdalenajadach self-assigned this Nov 23, 2023
@magdalenajadach magdalenajadach marked this pull request as ready for review November 23, 2023 17:39
@magdalenajadach magdalenajadach temporarily deployed to previews/a11y/773-Accessibility_replace_padding_values_with_logical_properties November 23, 2023 17:39 — with GitHub Actions Inactive
@github-actions GitHub Actions
Copy link

@github-actions GitHub Actions
Copy link

PetarSimonovic
PetarSimonovic reviewed Nov 24, 2023
View reviewed changes
Copy link
Contributor

@PetarSimonovic PetarSimonovic left a comment

Choose a reason for hiding this comment

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

HI Magda. All looks good - just one question! thanks

src/assets/stylesheets/AstroPiModel.scss
padding: $space-0-5;
padding-top: $space-0-25;
padding-inline: $space-0-5;
padding-block-start: $space-0-25;
box-sizing: border-box;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checking but will padding: $space-0-5 apply padding to top, right, bottom and left; then padding-top adds an extra 0-25 to the top?

so would this need to be:

padding-inline: $space-0-5;
padding-block-end: $space-0-5;
padding-block-start: $space-0-75;

Copy link
Contributor

Choose a reason for hiding this comment

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

I replaced that with correct values, thank you for spotting it

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks Magda. Does padding-block: $space-0-25 $space-0-5; need to be padding-block: $space-0-75 $space-0-5;?

I think the original applies $space-05 to all sides, then an additional $space-0-25 to the top

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 know where this $space-0-75 coming from? The original, as you mentioned, applies $space-05 to all sides and we have override the top with $space-0-25, which is the equivalent of padding-top: $space-0-25; padding-right: $space-0-5; padding-bottom: $space-0-5; padding-left: $space-0-5;
so it translates to padding-block: $space-0-25 $space-5; and padding-inline: $space-0-5;

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, perhaps I misunderstood. I didn't realise that padding-top overrides the padding: I thought it added to it!

@magdalenajadach magdalenajadach temporarily deployed to previews/a11y/773-Accessibility_replace_padding_values_with_logical_properties November 24, 2023 10:07 — with GitHub Actions Inactive
@github-actions GitHub Actions
Copy link

@github-actions GitHub Actions
Copy link

PetarSimonovic
PetarSimonovic approved these changes Nov 24, 2023
View reviewed changes
Copy link
Contributor

@PetarSimonovic PetarSimonovic left a comment

Choose a reason for hiding this comment

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

lgtm! 👍

@magdalenajadach magdalenajadach merged commit f11ec30 into main Nov 24, 2023
8 checks passed
@magdalenajadach magdalenajadach deleted the a11y/773-Accessibility_replace_padding_values_with_logical_properties branch November 24, 2023 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@magdalenajadach magdalenajadach magdalenajadach left review comments

@PetarSimonovic PetarSimonovic PetarSimonovic approved these changes

Assignees

@magdalenajadach magdalenajadach

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Accessibility: replace padding, position, and border values with logical properties
2 participants
@magdalenajadach @PetarSimonovic