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.

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

refactor(Breadcrumbs)!: use gap property for gaps between breadcrumbs items instead of paddings #1185

Merged
merged 2 commits into from
Jan 29, 2024

Conversation

isqua
Copy link
Contributor

@isqua isqua commented Dec 5, 2023

I use the Breadcrumbs component next to the other components, and would like the breadcrumbs to be aligned relative to the other components on the left edge. But the Breadcrumbs has a little gap on the left:

bc

It is because the padding property is used to create gaps between breadcrumbs items and diviers:

bc-layout

I also found an issue on this topic: #956

So I suggest to use the gap property for the parent of all items. This way there will be margins between the items, but the left and right items will not have an indentation.

Compare main branch preview and current branch preview

UPD: Please wait til I fix screenshot tests

@gravity-ui-bot
Copy link
Contributor

Preview is ready.

@isqua
Copy link
Contributor Author

isqua commented Jan 26, 2024

@korvin89 as you asked me in a chat, I tried to update tests for the component. But I faced an issue. I run the command npm run playwright:docker:update locally on my Apple M1, and it fails with the error:

RollupError: Could not resolve "./../../../path/to/gravity-ui/uikit/src/components/Avatar/__tests__/stories.ts" from "playwright/playwright/index.tsx"
More detailed log here
> @gravity-ui/[email protected] playwright:docker:update
> docker run --rm --network host -v $(pwd):/work/ -w /work/ -it mcr.microsoft.com/playwright:v1.38.1-jammy /bin/bash -c 'npm ci && npm run playwright:update'

> @gravity-ui/[email protected] playwright:update
> npm run playwright -- -u

> @gravity-ui/[email protected] playwright
> playwright test --config=playwright/playwright.config.ts -u

Running 56 tests using 1 worker

vite v4.4.11 building for production...
✓ 4 modules transformed.
✓ built in 881ms
Could not resolve "./../../../path/to/gravity-ui/uikit/src/components/Avatar/__tests__/stories.ts" from "playwright/playwright/index.tsx"
file: /work/playwright/playwright/index.tsx
RollupError: Could not resolve "./../../../path/to/gravity-ui/uikit/src/components/Avatar/__tests__/stories.ts" from "playwright/playwright/index.tsx"

    at error (/work/node_modules/rollup/dist/es/shared/node-entry.js:2287:30)
    at ModuleLoader.handleInvalidResolvedId (/work/node_modules/rollup/dist/es/shared/node-entry.js:24860:24)
    at ModuleLoader.resolveDynamicImport (/work/node_modules/rollup/dist/es/shared/node-entry.js:24920:58)
    at /work/node_modules/rollup/dist/es/shared/node-entry.js:24807:32

  56 skipped
transforming (26) ../node_modules/@storybook/client-api/dist/entry.mjs
To open last HTML report run:

  npx playwright show-report

In CI all tests passed:

  1 skipped
  55 passed (27.8s)

But the job failed because of missed token:

Input required and not supplied: GITHUB_TOKEN

What should be the next step to get this PR merged?

@korvin89
Copy link
Contributor

korvin89 commented Jan 26, 2024

Hey @isqua! Here is a related issue #1266

@korvin89 korvin89 changed the title refactor(Breadcrumbs): use gap property for gaps between breadcrumbs items instead of paddings; Fix #956 refactor(Breadcrumbs)!: use gap property for gaps between breadcrumbs items instead of paddings Jan 29, 2024
@korvin89 korvin89 merged commit 7c57c7f into gravity-ui:next Jan 29, 2024
3 of 4 checks passed
@isqua isqua deleted the breadcrumbs-gaps branch January 29, 2024 19:06
amje pushed a commit that referenced this pull request Feb 1, 2024
… items instead of paddings (#1185)

Co-authored-by: Mr.Dr.Professor Patrick <[email protected]>
amje pushed a commit that referenced this pull request Feb 6, 2024
… items instead of paddings (#1185)

Co-authored-by: Mr.Dr.Professor Patrick <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants