Skip to content

feat(ui5-side-navigation): migrate to TypeScript #6429

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

Merged
merged 13 commits into from
Feb 20, 2023
Merged

Conversation

kskondov
Copy link
Contributor

@kskondov kskondov commented Feb 6, 2023

Related to: #4337
Fixes: #6436

Fixed a bug where clicking on the fixedItems on a collapsed SideNavigation would not open the item's popover.

@kskondov kskondov requested review from a team, nnaydenow, ilhan007 and BorisDafov February 6, 2023 14:11
georgimkv

This comment was marked as outdated.

@kskondov
Copy link
Contributor Author

kskondov commented Feb 7, 2023

Please check if you can fix the errors about i18n texts being reported in the console: image

They also appear on the main branch, but if it's an easy fix we could include it.

I was not able to find a quick fix we might need to address this in another issue

@nnaydenow
Copy link
Contributor

nnaydenow commented Feb 9, 2023

Few changes:

Copy link
Contributor

@georgimkv georgimkv left a comment

Choose a reason for hiding this comment

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

IItem is a worse name. I prefer the previous name. Please also rebase.

@kskondov kskondov requested a review from georgimkv February 10, 2023 13:19
@georgimkv georgimkv self-assigned this Feb 14, 2023
subItems: Array<SideNavigationSubItem>,
};

type TSideNavigationItem = SideNavigationItem | SideNavigationSubItem;
Copy link
Member

Choose a reason for hiding this comment

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

I only would prefer to have ISideNavigationItem interface (instead of TSideNavigationItem), that both SideNavigationItem and SideNavigationSubItem implement

Copy link
Member

Choose a reason for hiding this comment

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

We discussed this comment offline and it won't help much to have an interface in this case.

@georgimkv georgimkv merged commit d8ab195 into main Feb 20, 2023
@georgimkv georgimkv deleted the TSSideNavigation branch February 20, 2023 12:38
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.

Side Navigation: Errors in console for i18n texts
4 participants