search

scania-digital-design-system / tegel

Tegel Design System
https://tegel.scania.com
MIT License
15 stars 12 forks source link

fix(stepper): correct first and last item padding #634

Closed adarean5 closed 1 month ago

adarean5 commented 1 month ago

Describe pull-request

Fixes an issue where the first and last styling was not applied to tds-step after the initial render of tds-stepper. The issue was that the styling was applied inside the componentWillLoad method of the tds-stepper. Now it's applied using pure CSS and no longer depends on the component lifecycle.

Fixes an issue where padding-left was applied to the first tds-step element and padding-right was applied to the last tds-step element in horizontal mode.

Issue Linking:

Choose one of the following options

How to test

  1. Run storybook
  2. Navigate to the stepper story
  3. Check the first element has no padding left in horizontal mode
  4. Check that the last element has no padding-right in the horizontal mode
  5. Check that the first element has no line before itself in both horizontal and vertical mode
  6. Check that the last element has no line after itself in both horizontal and vertical mode

Checklist before submission

Suggested test steps

sonarcloud[bot] commented 1 month ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

aws-amplify-eu-north-1[bot] commented 1 month ago

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-634.d3fazya28914g3.amplifyapp.com

adarean5 commented 1 month ago

Did not seem like it when I ran the tests locally since they all passed. Perhaps the tests only run for the vertical mode? @mJarsater could you run the tests locally yourself to verify?

mJarsater commented 1 month ago

Did not seem like it when I ran the tests locally since they all passed. Perhaps the tests only run for the vertical mode? @mJarsater could you run the tests locally yourself to verify?

I don't have a proper local setup so I can't validate right now. @theJohnnyMe wondering why the tests didn't run on push?

timrombergjakobsson commented 1 month ago

Did not seem like it when I ran the tests locally since they all passed. Perhaps the tests only run for the vertical mode? @mJarsater could you run the tests locally yourself to verify?

I ran the the tests locally in the branch and all passes!

adarean5 commented 1 month ago

Thank you for checking the tests @timrombergjakobsson . I'm not authorized to merge the PR, so feel free to merge it if you think no additional work is needed

mJarsater commented 1 month ago

@timrombergjakobsson @theJohnnyMe @nathalielindqvist @EmelieLitwin Not part of this PR but just wondering why the tests aren't running on push? Or am I missing something?

timrombergjakobsson commented 1 month ago

@timrombergjakobsson @theJohnnyMe @nathalielindqvist @EmelieLitwin Not part of this PR but just wondering why the tests aren't running on push? Or am I missing something?

Good question, they should run on push, @theJohnnyMe maybe you have some more insights?

timrombergjakobsson commented 1 month ago

@timrombergjakobsson @theJohnnyMe @nathalielindqvist @EmelieLitwin Not part of this PR but just wondering why the tests aren't running on push? Or am I missing something?

Good question, they should run on push, @theJohnnyMe maybe you have some more insights?

Or, btw, @mJarsater do you mean that the tests are not running for you when you push in a branch? Or did I misunderstand you?

adarean5 commented 1 month ago

@mJarsater I noticed the same, so I ran the tests manually. Perhaps there have been some changes to the hooks? If I remember correctly the tests used to run on the pre-commit hook, but I could be wrong.

timrombergjakobsson commented 1 month ago

@mJarsater I noticed the same, so I ran the tests manually. Perhaps there have been some changes to the hooks? If I remember correctly the tests used to run on the pre-commit hook, but I could be wrong.

Yes that is correct, however we changed to run on push, introduced here https://github.com/scania-digital-design-system/tegel/pull/624

mJarsater commented 1 month ago

@timrombergjakobsson @theJohnnyMe @nathalielindqvist @EmelieLitwin Not part of this PR but just wondering why the tests aren't running on push? Or am I missing something?

Good question, they should run on push, @theJohnnyMe maybe you have some more insights?

Or, btw, @mJarsater do you mean that the tests are not running for you when you push in a branch? Or did I misunderstand you?

Yeah exactly. I'm thinking the creation of this PR should have started a Github action that runs the tests?

timrombergjakobsson commented 1 month ago

@timrombergjakobsson @theJohnnyMe @nathalielindqvist @EmelieLitwin Not part of this PR but just wondering why the tests aren't running on push? Or am I missing something?

Good question, they should run on push, @theJohnnyMe maybe you have some more insights?

Or, btw, @mJarsater do you mean that the tests are not running for you when you push in a branch? Or did I misunderstand you?

Yeah exactly. I'm thinking the creation of this PR should have started a Github action that runs the tests?

Well actually the tests run when merging to develop. So not on creation of PR. Only on push and on merge. https://github.com/scania-digital-design-system/tegel/actions/workflows/playwright.yml

mJarsater commented 1 month ago

@timrombergjakobsson @theJohnnyMe @nathalielindqvist @EmelieLitwin Not part of this PR but just wondering why the tests aren't running on push? Or am I missing something?

Good question, they should run on push, @theJohnnyMe maybe you have some more insights?

Or, btw, @mJarsater do you mean that the tests are not running for you when you push in a branch? Or did I misunderstand you?

Yeah exactly. I'm thinking the creation of this PR should have started a Github action that runs the tests?

Well actually the tests run when merging to develop. So not on creation of PR. Only on push and on merge. https://github.com/scania-digital-design-system/tegel/actions/workflows/playwright.yml

Alright, that feels a bit risky. Might want to change it so it also runs on commit?

I mean we want to catch any issues before we merge things to develop?

timrombergjakobsson commented 1 month ago

@timrombergjakobsson @theJohnnyMe @nathalielindqvist @EmelieLitwin Not part of this PR but just wondering why the tests aren't running on push? Or am I missing something?

Good question, they should run on push, @theJohnnyMe maybe you have some more insights?

Or, btw, @mJarsater do you mean that the tests are not running for you when you push in a branch? Or did I misunderstand you?

Yeah exactly. I'm thinking the creation of this PR should have started a Github action that runs the tests?

Well actually the tests run when merging to develop. So not on creation of PR. Only on push and on merge. https://github.com/scania-digital-design-system/tegel/actions/workflows/playwright.yml

Alright, that feels a bit risky. Might want to change it so it also runs on commit?

I mean we want to catch any issues before we merge things to develop?

Yes it's a good point, we had before on commit, but created some friction so we changed it to push. As then you can at least commit a state, regardless of failing tests or not. I kinda like that, cos regardless we can't push if the tests fail. But maybe they should run also on opening of PR?

mJarsater commented 1 month ago

@timrombergjakobsson @theJohnnyMe @nathalielindqvist @EmelieLitwin Not part of this PR but just wondering why the tests aren't running on push? Or am I missing something?

Good question, they should run on push, @theJohnnyMe maybe you have some more insights?

Or, btw, @mJarsater do you mean that the tests are not running for you when you push in a branch? Or did I misunderstand you?

Yeah exactly. I'm thinking the creation of this PR should have started a Github action that runs the tests?

Well actually the tests run when merging to develop. So not on creation of PR. Only on push and on merge. https://github.com/scania-digital-design-system/tegel/actions/workflows/playwright.yml

Alright, that feels a bit risky. Might want to change it so it also runs on commit?

I mean we want to catch any issues before we merge things to develop?

Yes it's a good point, we had before on commit, but created some friction so we changed it to push. As then you can at least commit a state, regardless of failing tests or not. I kinda like that, cos regardless we can't push if the tests fail. But maybe they should run also on opening of PR?

Yeah it doesn't have to be in the pre-commit hook. But we should absolutely run in on commit.

theJohnnyMe commented 1 month ago

@timrombergjakobsson @theJohnnyMe @nathalielindqvist @EmelieLitwin Not part of this PR but just wondering why the tests aren't running on push? Or am I missing something?

Good question, they should run on push, @theJohnnyMe maybe you have some more insights?

Or, btw, @mJarsater do you mean that the tests are not running for you when you push in a branch? Or did I misunderstand you?

Yeah exactly. I'm thinking the creation of this PR should have started a Github action that runs the tests?

Well actually the tests run when merging to develop. So not on creation of PR. Only on push and on merge. https://github.com/scania-digital-design-system/tegel/actions/workflows/playwright.yml

Alright, that feels a bit risky. Might want to change it so it also runs on commit?

I mean we want to catch any issues before we merge things to develop?

Yes it's a good point, we had before on commit, but created some friction so we changed it to push. As then you can at least commit a state, regardless of failing tests or not. I kinda like that, cos regardless we can't push if the tests fail. But maybe they should run also on opening of PR?

Yeah it doesn't have to be in the pre-commit hook. But we should absolutely run in on commit.

Yes, we have moved running tests from pre-commit as it became pretty annoying as it takes 15sec or more to run tests and sometimes it fails for reasons not related to change one is doing. Runnin test now happens on pre-push hook. When you say to reintroduce it to commit do you think of running it here in GitHub once users pushes new commit to current PR?

mJarsater commented 1 month ago

@timrombergjakobsson @theJohnnyMe @nathalielindqvist @EmelieLitwin Not part of this PR but just wondering why the tests aren't running on push? Or am I missing something?

Good question, they should run on push, @theJohnnyMe maybe you have some more insights?

Or, btw, @mJarsater do you mean that the tests are not running for you when you push in a branch? Or did I misunderstand you?

Yeah exactly. I'm thinking the creation of this PR should have started a Github action that runs the tests?

Well actually the tests run when merging to develop. So not on creation of PR. Only on push and on merge. https://github.com/scania-digital-design-system/tegel/actions/workflows/playwright.yml

Alright, that feels a bit risky. Might want to change it so it also runs on commit?

I mean we want to catch any issues before we merge things to develop?

Yes it's a good point, we had before on commit, but created some friction so we changed it to push. As then you can at least commit a state, regardless of failing tests or not. I kinda like that, cos regardless we can't push if the tests fail. But maybe they should run also on opening of PR?

Yeah it doesn't have to be in the pre-commit hook. But we should absolutely run in on commit.

Yes, we have moved running tests from pre-commit as it became pretty annoying as it takes 15sec or more to run tests and sometimes it fails for reasons not related to change one is doing. Runnin test now happens on pre-push hook. When you say to reintroduce it to commit do you think of running it here in GitHub once users pushes new commit to current PR?

Exactly, so whenever you open a PR or commit new code to a PR.

theJohnnyMe commented 1 month ago

@timrombergjakobsson @theJohnnyMe @nathalielindqvist @EmelieLitwin Not part of this PR but just wondering why the tests aren't running on push? Or am I missing something?

Good question, they should run on push, @theJohnnyMe maybe you have some more insights?

Or, btw, @mJarsater do you mean that the tests are not running for you when you push in a branch? Or did I misunderstand you?

Yeah exactly. I'm thinking the creation of this PR should have started a Github action that runs the tests?

Well actually the tests run when merging to develop. So not on creation of PR. Only on push and on merge. https://github.com/scania-digital-design-system/tegel/actions/workflows/playwright.yml

Alright, that feels a bit risky. Might want to change it so it also runs on commit?

I mean we want to catch any issues before we merge things to develop?

Yes it's a good point, we had before on commit, but created some friction so we changed it to push. As then you can at least commit a state, regardless of failing tests or not. I kinda like that, cos regardless we can't push if the tests fail. But maybe they should run also on opening of PR?

Yeah it doesn't have to be in the pre-commit hook. But we should absolutely run in on commit.

Yes, we have moved running tests from pre-commit as it became pretty annoying as it takes 15sec or more to run tests and sometimes it fails for reasons not related to change one is doing. Runnin test now happens on pre-push hook. When you say to reintroduce it to commit do you think of running it here in GitHub once users pushes new commit to current PR?

Exactly, so whenever you open a PR or commit new code to a PR.

Yeah, that makes sense, will try to add it soon ;)

adarean5 commented 1 month ago

When it comes to running the tests, git hooks are nice, but can always be skipped with --no-verify, so I think it's a nice feature to have for some early feedback, but in my opinion, the tests should run in every PR as a part of the quality gates as @timrombergjakobsson suggested. This way any issues are fixed before the code is even merged into the develop branch. In an open-source model, this also means that the contributor of the PR will fix the failing tests before the changes are merged rather than the maintainers having to do it after the code has already been merged.

mJarsater commented 1 month ago

When it comes to running the tests, git hooks are nice, but can always be skipped with --no-verify, so I think it's a nice feature to have for some early feedback, but in my opinion, the tests should run in every PR as a part of the quality gates as @timrombergjakobsson suggested. This way any issues are fixed before the code is even merged into the develop branch. In an open-source model, this also means that the contributor of the PR will fix the failing tests before the changes are merged rather than the maintainers having to do it after the code has already been merged.

Yeah I think that's what everyone wants. I'm guessing @theJohnnyMe can take it to the team and see how this can be achieved in a good way.