Skip to content

Adaptive breadcrumb #4546

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
Merged
merged 3 commits into from
Jun 23, 2023
Merged

Adaptive breadcrumb #4546

merged 3 commits into from
Jun 23, 2023

Conversation

magdalenaan
Copy link
Contributor

@magdalenaan magdalenaan commented Jun 13, 2023 •
edited by Juveniel
Loading

@magdalenaan magdalenaan added Enhancement New feature of an existing functionality or an improvement of an existing functionality. C: Breadcrumb labels Jun 13, 2023
@magdalenaan magdalenaan self-assigned this Jun 13, 2023
@magdalenaan magdalenaan force-pushed the adaptive-breadcrumb branch from 9f5f531 to f158a30 Compare June 13, 2023 14:08
@magdalenaan magdalenaan marked this pull request as ready for review June 13, 2023 14:14
@magdalenaan magdalenaan requested a review from a team as a code owner June 13, 2023 14:14
@magdalenaan magdalenaan marked this pull request as draft June 14, 2023 06:42
@magdalenaan magdalenaan force-pushed the adaptive-breadcrumb branch from f3f8121 to b19f950 Compare June 14, 2023 08:23
@magdalenaan magdalenaan marked this pull request as ready for review June 14, 2023 08:25
inikolova
inikolova reviewed Jun 16, 2023
View reviewed changes
packages/fluent/scss/breadcrumb/_variables.scss Outdated
$kendo-breadcrumb-md-icon-link-padding-x: $kendo-breadcrumb-icon-link-padding-x !default;
/// The horizontal padding of the large Breadcrumb link icon.
/// @group breadcrumb
$kendo-breadcrumb-lg-icon-link-padding-x: $kendo-breadcrumb-lg-icon-link-padding-y !default;

// Root link spacing
$kendo-breadcrumb-root-link-spacing: 0px !default;
Copy link
Contributor

@inikolova inikolova Jun 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I believe we could remove this var (for all themes), as it is not necessary any more. According to the updated design, there is not padding set to the root item

@inikolova
Copy link
Contributor

inikolova commented Jun 16, 2023 •
edited
Loading

The Breadcrumb sizes look OK, Magi :) Just few more notes:

  • In some of the documented variables the first letter of the Breadcrumb is uppercase, in other it is lowercase (mostly in the Fluent theme). Could you please make all of them uppercase?
  • Breadcrumb adaptive rendering -> Is the rendering in the breadcrumb-adaptive.tsx the final one for adaptiveness (it looks pretty the same as the other tests)? Wouldn't the Breadcrumb be rendered inside an ActionSheet (link)?

Note: I checked with the designers team the changed height of the Breadcrumb (medium size) in the Fluent theme and confirm it is intentional.

@dmanova
Copy link

dmanova commented Jun 22, 2023

Breadcrumb adaptive rendering -> Is the rendering in the breadcrumb-adaptive.tsx the final one for adaptiveness (it looks pretty the same as the other tests)? Wouldn't the Breadcrumb be rendered inside an ActionSheet (link)?

@inikolova The final adaptive rendering is provided in the design and it doesn't include ActionSheet, just "hiding the middle items". If there is a demand for another UI, I've suggested a few new features to the Product managers, that may deserve attention here: https://github.com/telerik/web-components-ux/issues/799#issuecomment-1523702531

@magdalenaan magdalenaan requested a review from inikolova June 22, 2023 12:11
@magdalenaan magdalenaan force-pushed the adaptive-breadcrumb branch 2 times, most recently from d01091f to 3c94a91 Compare June 22, 2023 13:53
inikolova
inikolova reviewed Jun 23, 2023
View reviewed changes
packages/html/src/breadcrumb/tests/breadcrumb-size.tsx
<BreadcrumbContainer collapsing="wrap">
<BreadcrumbItem>
<Icon className="k-breadcrumb-delimiter-icon" icon="chevron-right" size="xsmall" />
<BreadcrumbLink focus>
Copy link
Contributor

Choose a reason for hiding this comment

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

The main purpose of this test is to demonstrate the different sizes of the Breadcrumb, therefore I would suggest to set background color to the page instead of using the items states (Like in this test:
https://github.com/telerik/kendo-themes/blob/develop/packages/html/src/bottom-nav/tests/bottom-nav.tsx)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Breadcrumb component should have a transparent background, so the sizes will not be visible changing the background of the test area.

@magdalenaan magdalenaan force-pushed the adaptive-breadcrumb branch 2 times, most recently from 105c8dc to afae9a2 Compare June 23, 2023 07:01
inikolova
inikolova reviewed Jun 23, 2023
View reviewed changes
packages/html/src/breadcrumb/tests/breadcrumb-adaptive.tsx Outdated
<BreadcrumbItem>
<Icon className="k-breadcrumb-delimiter-icon" icon="chevron-right" size="xsmall" />
<BreadcrumbLink>
<span className="test"></span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this class?

@magdalenaan magdalenaan requested a review from inikolova June 23, 2023 08:20
@magdalenaan magdalenaan force-pushed the adaptive-breadcrumb branch 2 times, most recently from 012f05c to 0d97176 Compare June 23, 2023 08:59
@magdalenaan magdalenaan force-pushed the adaptive-breadcrumb branch from e1ed9d2 to 5f124fe Compare June 23, 2023 09:55
@magdalenaan magdalenaan merged commit fa06e05 into develop Jun 23, 2023
@magdalenaan magdalenaan deleted the adaptive-breadcrumb branch June 23, 2023 11:33
@Juveniel Juveniel added this to the 2023.3 milestone Jun 26, 2023
This was referenced Jul 3, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@inikolova inikolova inikolova approved these changes

Assignees

@magdalenaan magdalenaan

Labels
C: Breadcrumb Enhancement New feature of an existing functionality or an improvement of an existing functionality.
Projects
None yet
Milestone
2023.3
Development

Successfully merging this pull request may close these issues.
5 participants
@magdalenaan @inikolova @dmanova @Juveniel @kendo-bot