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

fix(datagrid): align checkbox with text in datagrid #1006

Merged
merged 4 commits into from
Oct 18, 2023
Merged

fix(datagrid): align checkbox with text in datagrid #1006

merged 4 commits into from
Oct 18, 2023

Conversation

valentin-mladenov
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • If applicable, have a visual design approval

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Position of checkbox in data grid is not aligned with text.
image

Issue Number: CDE-1350

What is the new behavior?

Position of checkbox is centered with text's first line.
image

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@valentin-mladenov valentin-mladenov self-assigned this Oct 17, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 17, 2023 •
edited
Loading

👋 @valentin-mladenov,

  • 🙏 The Clarity team thanks you for opening a pull request
  • ⏳ The build for this PR has started
  • 📫 I'll update this comment when the build has finished

Thank you,

🤖 Clarity Release Bot

@github-actions
Copy link
Contributor

This PR introduces visual changes: a3bdd19
If these changes are intended and correct, please cherry-pick the above commit to this PR.

git checkout top-checkbox-datagrid_cde-1350
git fetch /~https://github.com/vmware-clarity/ng-clarity.git a3bdd19b12e79dd167524b6cde4d3f826a0027fc
git cherry-pick a3bdd19b12e79dd167524b6cde4d3f826a0027fc
git push

@valentin-mladenov valentin-mladenov changed the title fix: data grid checkbox position fix: align checkbox with text in datagrid Oct 17, 2023
williamernest
williamernest approved these changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

@williamernest williamernest left a comment

Choose a reason for hiding this comment

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

LGTM for the checkbox fix. Can you check if the radio button has the same issue and if this fix would work there as well ?

@williamernest williamernest changed the title fix: align checkbox with text in datagrid fix(datagrid): align checkbox with text in datagrid Oct 17, 2023
@valentin-mladenov
Copy link
Contributor Author

valentin-mladenov commented Oct 18, 2023 •
edited
Loading

@williamernest I cannot respond to your initial comment about radios directly.
Radio buttons are not affected, because are not used in datagrid header.
The fix is tightly related to checkbox specific setup in datagrid.

I can see radio buttons are maybe 1px off in top, but are like that originally.
image

williamernest
williamernest approved these changes Oct 18, 2023
View reviewed changes
Copy link
Contributor

@williamernest williamernest left a comment

Choose a reason for hiding this comment

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

Thanks for double-checking that for me. LGTM.

@valentin-mladenov valentin-mladenov merged commit 04a12dd into main Oct 18, 2023
@valentin-mladenov valentin-mladenov deleted the top-checkbox-datagrid_cde-1350 branch October 18, 2023 15:41
@valentin-mladenov valentin-mladenov mentioned this pull request Oct 19, 2023
Merged
13 tasks
valentin-mladenov added a commit that referenced this pull request Oct 19, 2023
@valentin-mladenov @web-flow
fix(datagrid): align checkbox with text in datagrid (port of #1006) (#…
a550051 
…1011)

## PR Checklist

Please check if your PR fulfills the following requirements:

- [ ] Tests for the changes have been added (for bug fixes / features)
- [ ] Docs have been added / updated (for bug fixes / features)
- [ ] If applicable, have a visual design approval

## PR Type

What kind of change does this PR introduce?

<!-- Please check the one that applies to this PR using "x". -->

- [x] Bugfix
- [ ] Feature
- [ ] Code style update (formatting, local variables)
- [ ] Refactoring (no functional changes, no api changes)
- [ ] Build related changes
- [ ] CI related changes
- [ ] Documentation content changes
- [ ] Other... Please describe:

## What is the current behavior?
Position of checkbox in data grid is not aligned with text.

![image](/~https://github.com/vmware-clarity/ng-clarity/assets/6692908/b876edd4-fed8-4804-ba8a-613296ed74af)

Issue Number: CDE-1350

## What is the new behavior?

Position of checkbox is centered with text's first line.

![image](/~https://github.com/vmware-clarity/ng-clarity/assets/6692908/e8e632ef-d186-4eb6-948d-8ab3c617fd11)

## Does this PR introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this PR contains a breaking change, please describe the impact
and migration path for existing applications below. -->

## Other information

---------

Co-authored-by: GitHub <noreply@github.com>
@github-actions
Copy link
Contributor

🎉 This PR is included in version 16.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

mivaylo pushed a commit that referenced this pull request Oct 25, 2023
@valentin-mladenov @web-flow @mivaylo
fix(datagrid): align checkbox with text in datagrid (port of #1006) (#…
2d16004 
…1011)

## PR Checklist

Please check if your PR fulfills the following requirements:

- [ ] Tests for the changes have been added (for bug fixes / features)
- [ ] Docs have been added / updated (for bug fixes / features)
- [ ] If applicable, have a visual design approval

## PR Type

What kind of change does this PR introduce?

<!-- Please check the one that applies to this PR using "x". -->

- [x] Bugfix
- [ ] Feature
- [ ] Code style update (formatting, local variables)
- [ ] Refactoring (no functional changes, no api changes)
- [ ] Build related changes
- [ ] CI related changes
- [ ] Documentation content changes
- [ ] Other... Please describe:

## What is the current behavior?
Position of checkbox in data grid is not aligned with text.

![image](/~https://github.com/vmware-clarity/ng-clarity/assets/6692908/b876edd4-fed8-4804-ba8a-613296ed74af)

Issue Number: CDE-1350

## What is the new behavior?

Position of checkbox is centered with text's first line.

![image](/~https://github.com/vmware-clarity/ng-clarity/assets/6692908/e8e632ef-d186-4eb6-948d-8ab3c617fd11)

## Does this PR introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this PR contains a breaking change, please describe the impact
and migration path for existing applications below. -->

## Other information

---------

Co-authored-by: GitHub <noreply@github.com>
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 4, 2023

Hi there 👋, this is an automated message. To help Clarity keep track of discussions, we automatically lock closed PRs after 14 days. Please look for another open issue or open a new issue with updated details and reference this one as necessary.

@github-actions github-actions bot locked and limited conversation to collaborators Nov 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@williamernest williamernest williamernest approved these changes

Assignees

@valentin-mladenov valentin-mladenov

Labels
cla-not-required released type:fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@valentin-mladenov @williamernest @vmwclabot @web-flow