fix(back-button, breadcrumb, item): flip chevron icons on RTL

[waelbettayeb]

Pull request checklist

Please check if your PR fulfills the following requirements:

• Tests for the changes have been added (for bug fixes / features)
• Docs have been reviewed and added / updated if needed (for bug fixes / features)
• Build (npm run build) was run locally and any changes were pushed
• Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

• Bugfix
• Feature
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no api changes)
• Build related changes
• Documentation content changes
• Other (please describe):

What is the current behavior?

Issue Number: #24729

What is the new behavior?

• back-button icon is flipped on RTL
• breadcrumb separator icon is flipped on RTL
• item detail icon is flipped on RTL

Does this introduce a breaking change?

• Yes
• No

Other information

[averyjohnston]

Thanks for the PR! For breadcrumb, this change makes sense, but this might break for back-button and item since backButtonIcon and detailIcon can be customized to whatever icon you want. (In which case it would be up to the developer to handle flipping behavior.)

Could we check if the icon is still the default before applying flip-rtl? In theory we could also do something like isRTL ? chevronForward : chevronBack, but that would require importing another icon that might not be used, which is a bit less performant.

[averyjohnston]

Can you also ensure there are tests for these changes? You can add to the existing screenshot tests for each component. item already has an RTL test (/item/test/basic/e2e.ts) so we would just need an item with detail turned on added to the HTML (/basic/index.html). For back-button and breadcrumb, you can add RTL tests to basic/e2e.ts, using item's as an example. (For breadcrumb, the tests are in /breadcrumbs.)

[averyjohnston]

Changing the default and type for this prop would be a breaking change, so we'd be unable to release it until v7. Could we do something like this instead when rendering? flip-rtl={detailIcon === chevronForward} (I'm open to cleaner alternatives, we just can't change the prop like this for now.)

[waelbettayeb]

Technically, I didn't change the default detail icon, I've added a custom function that resolves the icon.
The reason behind this decision is that when a developer chooses the detailIcon={chevronForward} it won't flip automatically.

[averyjohnston]

We're still changing the type of the prop, though, since it can now accept null. This will also change the auto-generated docs, which pull in the default assigned here.

If the dev assigns their own icon, they can use custom icons for each case, something like: detailIcon={document.dir === 'rtl' ? chevronBack : chevronForward}

[averyjohnston]

Can you also add in the tests I described in my previous comment? Let me know if you have any questions there.

[averyjohnston]

Thanks for the updates! We'll still need tests added to make sure this does not regress in the future. I'll also add the team back in for review to get another pair of eyes on it.

[averyjohnston]

Thanks! This LGTM, should be good to merge once we have one more reviewer.

[sean-perkins]

This looks good to me. I've updated your original PR template to link to the related issue as well as updated the PR title to match our commit message standards.

We will just need to make sure when squashing + merging, that that title is used for the subject and that Resolves # is used in the description line.