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

Modal layout unification #758

Closed
wants to merge 11 commits into from
Closed

Modal layout unification #758

wants to merge 11 commits into from

Conversation

AllienWorks
Copy link
Member

@AllienWorks AllienWorks commented Jan 24, 2018

WIP PR containing experiments and debug for the modals' layout.
Closes #694, #753

  • I've stripped down a lot (close buttons, styles etc.)
  • there is a bug in former "big" modals - when closing, they somehow get smaller to a small square (that's because of the padding there) and this little square disappears then.. I'm not sure why other modals (the current smaller dialogs) don't behave like that.. can someone investigate?

@AllienWorks
Copy link
Member Author

Tried to disable animations (https://material.angular.io/guide/getting-started#step-2-animations), after that I can't reproduce the issue. Can @vikas-cis and @anandsinghparihar confirm?

@vikas-cis-zz
Copy link

@AllienWorks it seems the issue is not appearing but the modal looks ugly to me

Copy link
Contributor

@anandsinghparihar anandsinghparihar left a comment

Choose a reason for hiding this comment

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

@AllienWorks its working fine in mac & Linux now, you removed all animation from app its not look good.

@AllienWorks
Copy link
Member Author

So after reading a lot of different issues in angular/material and related, I found a lot of other devs (1, 2, 3) having the same issues as we have (moved elements or their parts).

Looks like it's a problem with Electron and not Angular Material, apparently after updating to latest Electron (1.8.2) beta everything works as expected (source).

Can we try that? At least in some testing branch (like this one)? Would be great to pin point the problem, so at least we know where it originates. We could focus on other issues and when Electron 1.8.2 is stable and out, simple update will hopefully do.

@AllienWorks
Copy link
Member Author

I've unified the modals look and code.

The situation
So, the Material dialogs/modals are built by 3 parts. The title (mat-dialog-title), content (mat-dialog-content) and optionally some action buttons (mat-dialog-actions). Currently, some of the modals (known as "small" dialogs, e.g. Add new address to Address book, Address lookup, Console, QR modal) are using exactly this setup. So that's good.

Then we have the "big" modals (e.g. Create wallet, Encrypt wallet, Cold staking), which are built with the modals.component. However in that component, we have just one "place" where we insert content (think mat-dialog-content). Therefore everything these modals are made of is inserted to "modal's content". It doesn't sound bad, but we can't have proper titles and/or the action buttons there – if you insert those in the content itself (and not where they belong – mat-dialog-title and mat-dialog-actions), it's breaking the layout. You can try to pull this PR and see it action in the Encrypt/Cold staking modals.

This is all related to remake of modals (we have to do it either way). The issue with "jumping content" should be resolved by updating Electron to 1.8.2 as I wrote here earlier, just FYI.

What needs to be done
IF we want to use templates (and I think that's surely a good idea; let's not repeat ourselves), we need to build one master template (think modals.component), BUT we need to include all of the 3 parts there – so we can inject all types of content there (title, content and action buttons).

I'm not quite sure if it's possible, but it should be. Can someone look into it? I would start by adapting the modals.component (or you can create a new one, e.g. dialogs.component) and then implement it into coldstake.component (that one is easy enough).

@kewde
Copy link
Collaborator

kewde commented Jan 28, 2018

I'd like to focus on moving the "big modals" out as much as possible and then fix the ones that are left..
I haven't looked very deep into the modals but it shouldn't be too hard, we plan some time next week to fix this if possible?

@AllienWorks
Copy link
Member Author

@kewde absolutely. Let me know, we'll work it out.

@AllienWorks AllienWorks changed the title modal layout debug (WIP) Modal layout unification Jan 29, 2018
@pciavald pciavald added this to the 1.1.1 milestone Jan 29, 2018
@AllienWorks
Copy link
Member Author

@vikas-cis @anandsinghparihar can you test again, if the "jumping content" bug is still present? Thanks!

@vikas-cis-zz
Copy link

screen shot 2018-01-29 at 8 10 13 pm

@AllienWorks it seems when I try to open cold staking modal from widget 4-5 times and then I try 6th time then it stuck not sure why ?

@vikas-cis-zz
Copy link

@AllienWorks check boxes for coldstaking widget and unlock modal are still moving as shared you via screenshare

Thanks

@AllienWorks
Copy link
Member Author

@vikas-cis the screen you shared is the result of all I wrote above. We need to rework the modals. Already on that with @kewde.

@anandsinghparihar
Copy link
Contributor

anandsinghparihar commented Jan 29, 2018

Edit by @AllienWorks: (fixed)

@pciavald pciavald changed the base branch from dev to develop February 3, 2018 00:09
@rynomster
Copy link
Member

Are we still going to need this?

@AllienWorks
Copy link
Member Author

@rynomster yes, absolutely. We need to rework the modals altogether. Current setup isn't done properly (speaking about layout).

@AllienWorks
Copy link
Member Author

AllienWorks commented Jun 1, 2018

Already solved in #1025

@AllienWorks AllienWorks deleted the fix/modals-layout branch June 12, 2018 13:08
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.

Modal design issue in Linux build
6 participants