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

[Fix] Move LanguageMenu to core #490

Merged
merged 5 commits into from
Jul 28, 2021
Merged

Conversation

ketsappi
Copy link
Contributor

Description

  • LanguageMenu is now moved to core level
  • Use object style for CSS class names to prevent duplicated content that are more prone to error
  • Directory structure added for LanguageMenu
    • Some components and props were renamed as it was possible to drop unwanted abstraction with folder structure rethinking.
  • Whole components level is removed from the library as nothing were left after the removal of this last component.

This PR is continuing the job done in #488.

Motivation and Context

Removing the components levels abstraction will remove unwanted complexity. This will possibly increase the speed of making changes or new features.

How Has This Been Tested?

Locally in styleguidist and in CRA-TS project.

Release notes

Breaking changes

  • LanguageMenuItemLanguage, LanguageMenuItemLanguageProps, LanguageMenuLinkLanguage, LanguageMenuLinkLanguageProps removed. Use LanguageMenuItem, LanguageMenuItemProps, LanguageMenuLink, LanguageMenuLinkProps instead.
  • Statics languageItem, LinkLanguage removed from LanguageMenu. Use LanguageMenuItem or LanguageMenuLink instead.

@ketsappi ketsappi added the DX Refactoring for maintainability label Jul 13, 2021
@ketsappi ketsappi requested review from aappoalander and LJKaski July 13, 2021 07:49
Comment on lines 8 to 6
export interface LanguageMenuLinkLanguageProps extends LanguageMenuLinkProps {
export interface LanguageMenuItemProps extends LanguageMenuItemBaseProps {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that GitHub shows wrong files in the compare, because of the folder structure change and rename. Also same case with the LanguageMenuLink file below.

Comment on lines -57 to -60
LanguageMenuItemLanguage,
LanguageMenuItemLanguageProps,
LanguageMenuLinkLanguage,
LanguageMenuLinkLanguageProps,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are no longer exported as those can be used directly from LanguageMenuItem, LanguageMenuItemProps and LanguageMenuLink, LanguageMenuLinkProps.

export type LanguageMenuPopoverItemsProps =
| LanguageMenuItemBaseProps
| LanguageMenuLinkPropsWithType;
type OptionalLanguageMenuPopoverProps = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could probably be written as Partial, but should not make any difference if left as is.

@aappoalander
Copy link
Contributor

Looks good! This component could be simplified further, because it supports unnecessary use cases like custom props and components as pros instead of using composition. These improvements should be implemented later on as a separate PR, but I decided to comment here anyways.

@aappoalander
Copy link
Contributor

Tested with all Mac OS setups (SSR test project, CRA-TS, Styleguidist and DS-site) using Brave, Safari, Firefox and Edge with VO on. Seems to be working as expected.

@aappoalander aappoalander merged commit 50adae8 into develop Jul 28, 2021
@ketsappi ketsappi deleted the fix/move-languagemenu-to-core branch August 17, 2021 05:03
@aappoalander aappoalander mentioned this pull request Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX Refactoring for maintainability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants