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

hotfix/mobile-menu-alignment #2333

Closed

Conversation

PensoGlide
Copy link
Contributor

Description:
This PR solves the issues where the "Policy Viewer" and the "Policy Editor" submenus would overlap with the rest of the menu on mobile.

Solves issue #2064 .

@PensoGlide PensoGlide force-pushed the hotfix/mobile-menu-alignment branch from e1be278 to ab97cd5 Compare June 26, 2023 12:54
@PensoGlide PensoGlide marked this pull request as ready for review June 26, 2023 13:19
@ieumuzair ieumuzair added the IEU Intellect EU Team label Jun 26, 2023
mobileMenuFix(): void {
if (this.innerWidth <= 810) {
setTimeout(function () {
window.location.reload();
Copy link
Collaborator

Choose a reason for hiding this comment

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

location.reload is bad practice, need normal fix using css styles

@PensoGlide PensoGlide force-pushed the hotfix/mobile-menu-alignment branch from ab97cd5 to 0a975dd Compare June 26, 2023 18:18
@simvalery
Copy link
Collaborator

simvalery commented Jun 27, 2023

Screenshot_20230627-212021_Chrome
it does not work

@PensoGlide PensoGlide force-pushed the hotfix/mobile-menu-alignment branch from 0a975dd to 4785f2c Compare June 27, 2023 22:52
@Stepan-Kirjakov
Copy link
Collaborator

  • Using JS for animation and hiding elements is not great especially if it can be done via css. Using css for such task is ‘best practice’ because it would be more maintainable and error free.
  • Direct changes of the DOM model in angular are dangerous, this can affect the stability and correctness of angular itself. If it is completely unavoidable (which is not in this case) standard angular methods should be used for that.

I have implemented a fix for one of the ticket issues (d73bf6b), this is an example of the recommended approach - please take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IEU Intellect EU Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants