Skip to content

Conversation

sKudum
Copy link
Contributor

@sKudum sKudum commented May 21, 2021

Related Issue

Closes #2262

Description

BREAKING CHANGE:
Added a new modifier class to fd-row to achieve even alignment of its elements:fd-row--top in a form.

Form Grid aligns items vertically to the center when according to the guideline it should align to the top.

When you have more than one column and the number of the fields is not equal for each column fields are aligned to the middle instead of to the top:

image

According to the guideline it should be aligned to the top:

image

Screenshots

Before:

image

After:

image

Please check whether the PR fulfills the following requirements

  1. The output matches the design specs
  • All values are in rem
  • Text elements follow the truncation rules
  • hover state of the element follow design spec
  • focus state of the element follow design spec
  • active state of the element follow design spec
  • selected state of the element follow design spec
  • selected hover state of the element follow design spec
  • pressed state of the element follow design spec
  • Responsiveness rules - the component has modifier classes for all breakpoints
  • Includes Compact/Cosy/Tablet design
  • RTL support
  1. The code follows fundamental-styles code standards and style
  • only one top level fd-* class is used in the file
  • BEM naming convention is used
  • Mixins are used for repeatable code (fd-rtl, fd-ellipsis, fd-flex, fd-selected, fd-focus, ect.)
  • A11y support - keyboard support, screenreader support, proper ARIA attributes, etc.
  • fd-reset() mixin is applied to all elements
  • Variables are used, if some value is used more than twice.
  • Checked if current components can be reused, instead of having new code.
  1. Testing
  • tested Storybook examples with "CSS Resources" normalize option
  • tested Storybook examples with "CSS Resources" unnormalize option
  • Verified all styles in IE11
  • Updated tests
  • last commit message should have [ci visual] so it can trigger chromatic visual regression (e.g. test: run chromatic visual regression [ci visual])
  1. Documentation
  • Storybook documentation has been created/updated
  • Breaking Changes wiki has been updated in case of breaking changes.

@sKudum sKudum requested review from Betrozov and DeepakSap14 May 21, 2021 12:34
@netlify
Copy link

netlify bot commented May 21, 2021

✔️ Deploy Preview for fundamental-styles ready!

🔨 Explore the source changes: 1991ceb

🔍 Inspect the deploy log: https://app.netlify.com/sites/fundamental-styles/deploys/60bf3a1c2881c60007c4ffe1

😎 Browse the preview: https://deploy-preview-2371--fundamental-styles.netlify.app/iframe

@sKudum sKudum force-pushed the fix/2262-grid-alignment branch from bd5e4ae to a6c0577 Compare May 21, 2021 13:02
@sKudum sKudum requested a review from InnaAtanasova May 21, 2021 13:46
@sKudum sKudum mentioned this pull request May 25, 2021
@sKudum sKudum linked an issue May 25, 2021 that may be closed by this pull request
@InnaAtanasova InnaAtanasova added the Bug Something isn't working label May 25, 2021
@InnaAtanasova InnaAtanasova added this to the Sprint 63 - Ariba milestone May 25, 2021
@InnaAtanasova InnaAtanasova self-requested a review May 25, 2021 17:20
Copy link
Contributor

@InnaAtanasova InnaAtanasova left a comment

Choose a reason for hiding this comment

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

@sKudum this change is introducing another bug. Now the label and the input are not aligned vertically.
Before:
Screen Shot 2021-05-25 at 1 20 32 PM

Now:
Screen Shot 2021-05-25 at 1 20 00 PM

@sKudum sKudum force-pushed the fix/2262-grid-alignment branch from a6c0577 to 6a9e105 Compare May 26, 2021 15:02
@sKudum sKudum requested a review from InnaAtanasova May 26, 2021 15:02
Copy link
Contributor

@InnaAtanasova InnaAtanasova left a comment

Choose a reason for hiding this comment

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

baseline is not centering vertically the label and the input field:
Screen Shot 2021-05-26 at 1 47 50 PM
Screen Shot 2021-05-26 at 1 47 45 PM

@sKudum
Copy link
Contributor Author

sKudum commented May 28, 2021

baseline is not centering vertically the label and the input field:
Screen Shot 2021-05-26 at 1 47 50 PM
Screen Shot 2021-05-26 at 1 47 45 PM

Hi @InnaAtanasova ,
When explored came to know that for horizontal and compact line-hight are causing issues with alignment of label with elements, To overcome it created a modifiers for both of them with basic heights.
Class and examples are updated accordingly with details.

Please let me know if this approach is acceptable or not, If not then please let me know the suggested approach.

Thank You

@sKudum sKudum requested a review from InnaAtanasova May 28, 2021 17:52
@sKudum sKudum force-pushed the fix/2262-grid-alignment branch 4 times, most recently from d4c0e3b to a0e2f78 Compare June 1, 2021 14:51
@sKudum sKudum requested review from InnaAtanasova and a team June 3, 2021 13:34
@sKudum sKudum force-pushed the fix/2262-grid-alignment branch from 7db930d to b3f5a6b Compare June 3, 2021 15:00
@sKudum sKudum requested a review from platon-rov June 4, 2021 12:55
@sKudum sKudum force-pushed the fix/2262-grid-alignment branch from b3f5a6b to 0a2b060 Compare June 4, 2021 14:14
@InnaAtanasova
Copy link
Contributor

@sKudum I think you need to update the images for the tests. Some of them show the label not aligned in the middle. I think it's from the prev commit. You can see the screenshots I denied.

@sKudum sKudum force-pushed the fix/2262-grid-alignment branch from 0a2b060 to 45a2e30 Compare June 4, 2021 18:00
@sKudum
Copy link
Contributor Author

sKudum commented Jun 4, 2021

@sKudum I think you need to update the images for the tests. Some of them show the label not aligned in the middle. I think it's from the prev commit. You can see the screenshots I denied.

Hi @InnaAtanasova ,

As per observation and doing reliazed we don't need to add form-label--horizontal or form-label--compact classes instead added fd-row--top class to fix this issue by which we can avoid breaking any changes. We can remove breaking change from this PR.

Please let me know your view on it.Will update accordingly.

@sKudum sKudum requested a review from InnaAtanasova June 4, 2021 18:03
@sKudum sKudum force-pushed the fix/2262-grid-alignment branch 2 times, most recently from 6f6bac2 to 8bfdf93 Compare June 7, 2021 07:44
@InnaAtanasova
Copy link
Contributor

@sKudum I think you need to update the images for the tests. Some of them show the label not aligned in the middle. I think it's from the prev commit. You can see the screenshots I denied.

Hi @InnaAtanasova ,

As per observation and doing reliazed we don't need to add form-label--horizontal or form-label--compact classes instead added fd-row--top class to fix this issue by which we can avoid breaking any changes. We can remove breaking change from this PR.

Please let me know your view on it.Will update accordingly.

It's def a better solution. Thanks for digging more. You still need to add the Breaking changes, tho. It's a new modifier class that needs to be reflected in the implementation libraries.

@sKudum
Copy link
Contributor Author

sKudum commented Jun 7, 2021

@sKudum I think you need to update the images for the tests. Some of them show the label not aligned in the middle. I think it's from the prev commit. You can see the screenshots I denied.

Hi @InnaAtanasova ,
As per observation and doing reliazed we don't need to add form-label--horizontal or form-label--compact classes instead added fd-row--top class to fix this issue by which we can avoid breaking any changes. We can remove breaking change from this PR.
Please let me know your view on it.Will update accordingly.

It's def a better solution. Thanks for digging more. You still need to add the Breaking changes, tho. It's a new modifier class that needs to be reflected in the implementation libraries.

Hi @InnaAtanasova , Updated breaking change information.

@manjunathanagaraj manjunathanagaraj removed this from the Sprint 63 - Ariba milestone Jun 8, 2021
sKudum added 3 commits June 8, 2021 15:05
fix: addressing inna review comments [ci visual]

fix: addressing compact horizontal lablel alignment [ci visual]

fix: addressing build issues [ci visual]

fix: build issues2 [ci visual]
fix: rebase with main [ci visual]
@sKudum sKudum force-pushed the fix/2262-grid-alignment branch from 8bfdf93 to 1991ceb Compare June 8, 2021 09:36
@DeepakSap14 DeepakSap14 changed the title fix: Form Grid aligns items vertically to the top fix: form grid aligns items vertically to the top Jun 10, 2021
@sKudum sKudum merged commit 848f09e into main Jun 10, 2021
@sKudum sKudum deleted the fix/2262-grid-alignment branch June 10, 2021 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Form Grid alignment
6 participants