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

feat(admin-ui): Add rtl compatibility to some admin-ui components #2451

Merged
merged 6 commits into from
Oct 17, 2023
Merged

Conversation

HoseinGhanbari
Copy link
Contributor

ModalDialogComponent is now supporting dynamic direction (ltr/rtl) based on the language & locale provided by dataService.

@HoseinGhanbari
Copy link
Contributor Author

DropdownMenuComponent is now supporting dynamic direction (ltr/rtl) based on the language & locale provided by dataService.

@HoseinGhanbari
Copy link
Contributor Author

ThemeSwitcherComponent is now compatible with rtl layouts.

@HoseinGhanbari HoseinGhanbari changed the title feat(admin-ui): Support dynamic ltr/rtl on ModalDialogComponent feat(admin-ui): Add rtl compatibility to some admin-ui components Oct 14, 2023
@HoseinGhanbari
Copy link
Contributor Author

NotificationComponent is now supporting dynamic direction (ltr/rtl) based on the language & locale provided by dataService.

@HoseinGhanbari
Copy link
Contributor Author

NotificationService's unit tests using ng test can now be completed successfully without any failures.

@michaelbromley
Copy link
Member

Hi,
Thanks for this contribution! I have an idea about how we can simplify this: the current ui language is always stored in local storage and thus it should be possible to be retrieved from the affected components using:

export class NotificationComponent implements OnInit {
  direction: 'ltr' | 'rtl';

  constructor(private localStorageService: LocalStorageService, private i18nService: I18nService) {
    this.direction = this.i18nService.isRTL(this.localStorageService.get('uiLanguageCode');
  }

}

Can you try to refactor like this and see if that works? That would cut down on the amount of code we need to add quite significantly.

@HoseinGhanbari
Copy link
Contributor Author

Yeah, I'll give it a shot. but I think there may be cases that setting the direction inside constructor wouldn't take effect when user try change the locale/language at runtime/asynchronously. Actually we have to update the direction based on the user interactions and on an observable basis. I will keep you updated. But let me know if you are ok with setting direction inside constructor.

Also note that, as an alternate solution, we can create some sort of BehaviorSubject on a shared service that every component which requires awareness of current ltr / rtl status can subscribe and do the rest inside their ts file.

@michaelbromley
Copy link
Member

My thinking was that since the language can only be set in 1 place (the language dialog) then it might be ok to assume a static value for a given component. But is this causes edge-cases then my 2nd choice would indeed be to encapsulate the observable logic in a shared service.

@HoseinGhanbari
Copy link
Contributor Author

Thank you for your reply. I will soon update the strategy on accessing direction, refactor the whole changes and keep you updated. 🚀

@HoseinGhanbari
Copy link
Contributor Author

  • new LocalizationService with its minimal unit test has been added.
  • both uiLanguageAndLocale$ & direction$ has been moved to LocalizationService.
  • new shared types called LocalizationLanguageCodeType & LocalizationDirectionType has been extracted.
  • all usages of uiLanguageAndLocale$ & direction$ is now referencing LocalizationService.
  • small scss & autoprefixer warning related to theme-switcher.component.scss has been resolved.
  • access to ltr/rtl status is now as easy as the following.
import { LocalizationDirectionType, LocalizationService } from '../../providers/localization/localization.service';

export class NotificationComponent implements OnInit {
    direction$: LocalizationDirectionType;

    constructor(private localizationService: LocalizationService) { }

    ngOnInit(): void {
        this.direction$ = this.localizationService.direction$;
    }
}

@michaelbromley michaelbromley merged commit 96eb96e into vendure-ecommerce:minor Oct 17, 2023
9 checks passed
@michaelbromley
Copy link
Member

Beautiful refactor, thank you!

@HoseinGhanbari
Copy link
Contributor Author

Its my pleasure! 👌

@HoseinGhanbari HoseinGhanbari deleted the minor branch October 17, 2023 14:53
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.

2 participants