Generator-jhipster: Remove Popup from CRUD navigation

[azizkhani]

Hi, i am using JHipster and think current navigation make JHipster code complex and management of code is very hard ,one of big problem in business application is change management and upgrade to new feature .
I suggest change navigation to this way.
When user click on view or edit button .route change and component render in default outlet instead of open in popup .
When user click on delete button .confirm dialog open and after user select ok .entity remove and grid reload;
This gift show this navigation .

In this navigation .all pop component and service remove and create on resolver for load data in resolver;

Current navigation

Disadvantage:

1. Complex code
2. More Code
3. High cohesion: change propert outside of component using componentInstance in (entity-detail.component.ts and entity-dialog.component.ts)
4. Popup dose not good for a lot of input and complex form

what is opinion of jhipster team about this navigation and advantage and disadvantage of this scenario?

[gmarziou]

Personally, I don't see the benefit of popups for editing or viewing details and I think they have drawbacks in terms of accessibility, so I'm in favor of getting rid of them as proposed by @azizkhani and have simpler routing.

[deepu105]

Ya probably its time to get rid of models for edit and create. But for
delete its needed. It will remove a lot of boilerplate for entities that we
added since Angular router doesn't play very nice with dialog models

On Sat, 24 Mar 2018, 4:28 pm Gaël Marziou, [e-mail] wrote:

Personally, I don't see the benefit of modals for editing or viewing
details, so I'm in favor of getting rid of it as proposed by @azizkhani
https://github.com/azizkhani

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/jhipster/generator-jhipster/issues/7352#issuecomment-375897891,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABDlF-gwbJarBb8lBT45Sjl2np06NSS-ks5thmYWgaJpZM4S5xVd
.

[jdubois]

Oh yes, I've also never liked the modals... but do we have time to refactor this before JHipster 5?

[gmarziou]

@deepu105 is it done also with modals in React?

@azizkhani would you like to contribute?

[azizkhani]

@gmarziou yes i like that ,but i dont know how will do it .

[rasoolghafari]

Dear @azizkhani that's right, it's very annoying design for navigation

[gzsombor]

Yes, modals are a bit annoying for the back button, but it's simpler to put the same modal dialog to different pages, and communicate with the "host" page, after the modal is finished.

[gmarziou]

@azizkhani you could clone the generator repo, start from master branch and follow our guide. All work should be in https://github.com/jhipster/generator-jhipster/tree/master/generators/entity-client/templates/angular/src

We will help you.

[jdubois]

So I think we all agree about this, but unless there's an easy way to do it, we won't merge this for JHipster v5 - this leaves time to do it well, and @azizkhani if you want to do it, you are most welcome
(and this needs to be done for Angular and React)

[deepu105]

For React I'll probably do it as its simple anyway, for angular it would be
a breaking change, at least in terms of URL and stuff, so if doing I would
propose to do it for JH5, anyway, it shouldn't be such a huge change, I
would have done it if I had the time, but if someone is up for it i'll
review and merge it

Thanks & Regards,
Deepu

On Sun, Mar 25, 2018 at 7:33 PM Julien Dubois [e-mail]
wrote:

So I think we all agree about this, but unless there's an easy way to do
it, we won't merge this for JHipster v5 - this leaves time to do it well,
and @azizkhani https://github.com/azizkhani if you want to do it, you
are most welcome
(and this needs to be done for Angular and React)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/jhipster/generator-jhipster/issues/7352#issuecomment-375988175,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABDlF2hga9wS6rpPjNWzI8jJ6k7mUSQZks5th9TcgaJpZM4S5xVd
.

[jdubois]

Oh if you can do it easily @deepu105 do go on, but we have 40+ opened bugs, and I'd like to make our beta release

[deepu105]

Dont hold up beta for this

Thanks & Regards,
Deepu

On Sun, Mar 25, 2018 at 7:40 PM Julien Dubois [e-mail]
wrote:

Oh if you can do it easily @deepu105 https://github.com/deepu105 do go
on, but we have 40+ opened bugs, and I'd like to make our beta release

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/jhipster/generator-jhipster/issues/7352#issuecomment-375988737,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABDlF19GkaC4dJ4dUtHVvW0DSvpjEAHoks5th9amgaJpZM4S5xVd
.

[azizkhani]

@gmarziou @deepu105 @jdubois I'm so happy to be able to do this with your help and I hope to do it well.I was worried about not being familiar with the process and time of doing it. It is good news that you won't merge this for JHipster v5 ,and I have time to do the right thing .

[azizkhani]

Is not it better? First, I'll write an example with this navigation and we agree about that and then start applying to generator?

[gmarziou]

OK, let's do it this way: start from a simple app, make your changes as a single commit that could be discussed.

[azizkhani]

@gmarziou I changed label navigation and push to https://github.com/azizkhani/jhipster-sample-app,
please check that .

[gmarziou]

Great work @azizkhani ! I tested the web app interactively and I like the new navigation.

Minor thing: yarn test fails on tslint and karma.

[deepu105]

Can you do the same on the template and PR to this repo?

Few things to note,

get rid of all Model related code for the edit/create pages since its not
needed anymore (keep it only for delete)
Make sure karma tests and e2e tests are working

Thanks & Regards,
Deepu

On Tue, Mar 27, 2018 at 12:14 AM Gaël Marziou [e-mail]
wrote:

Great work @azizkhani https://github.com/azizkhani ! I tested the web
app interactively and I like the new navigation.

Minor thing: yarn test fails on tslint and karma.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/jhipster/generator-jhipster/issues/7352#issuecomment-376329367,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABDlF5VYSDdRv3FuSmsE2f3SyOlhxAe9ks5tiWg0gaJpZM4S5xVd
.

[azizkhani]

Yes I can do it ,But need help ,Now after run yarn test . i found will change karma test .
https://stackoverflow.com/questions/49506086/create-test-for-activatedroute-data

[deepu105]

What kind of help do you need?

Thanks & Regards,
Deepu

On Tue, Mar 27, 2018 at 8:56 AM Ali Akbar Azizkhani <
[email protected]> wrote:

Yes I can do it ,But need help

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/jhipster/generator-jhipster/issues/7352#issuecomment-376417579,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABDlF8ypDmRQTn2srOCAhSfDLNqXqXb1ks5tieKSgaJpZM4S5xVd
.

[azizkhani]

Answer to some question during do that.

[deepu105]

@azizkhani ofcourse, you can ping me if you have any questions. I will also review your PR

[azizkhani]

@deepu105 I change that for entity and push in this repository.
https://github.com/azizkhani/generator-jhipster
will i send a PR or you can see my change before that?

[deepu105]

Please send a PR

Thanks & Regards,
Deepu

On Wed, Mar 28, 2018 at 5:25 PM Ali Akbar Azizkhani <
[email protected]> wrote:

@deepu105 https://github.com/deepu105 I change that for entity and push
in this repository.
https://github.com/azizkhani/generator-jhipster
will i send a PR or you can see my change before that?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/jhipster/generator-jhipster/issues/7352#issuecomment-376927275,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABDlF79OEycCRzUrE9_rmB10d5_F2e7cks5ti6tTgaJpZM4S5xVd
.

[gmarziou]

@azizkhani a PR will trigger our Travis builds, so it is very useful for testing many different configurations though it takes some time to run.

[azizkhani]

@deepu105 @gmarziou I sent a PR

[gmarziou]

I saw it :)
One thing you could do in your PR comment is to add Fix #7352 on a line so that your PR is linked to this issue and also once merged it will close it.

[azizkhani]

@gmarziou how can i test in local instead of push and see travis?

[gmarziou]

@azizkhani see https://github.com/jhipster/generator-jhipster/blob/master/CONTRIBUTING.md#local-travis-build

[gmarziou]

Not sure yet but the upgrade generator could be in trouble with the renaming of the dialog components. I'll try on my project as soon as a beta is released.

[hejerdj]

Hi all, i'am a beginner in jhipster. I modify the files like @azizkhani but when i run my app it shows me an error and i don't understand what the source of the problem.

BasicinfoDialogComponent.html:1 ERROR TypeError: Cannot read property 'id' of undefined
at Object.eval [as updateDirectives] (BasicinfoDialogComponent.html:1)
at Object.debugUpdateDirectives [as updateDirectives] (core.js?593e:14640)
at checkAndUpdateView (core.js?593e:13787)
at callViewAction (core.js?593e:14138)
at execComponentViewsAction (core.js?593e:14070)
at checkAndUpdateView (core.js?593e:13793)
at callViewAction (core.js?593e:14138)
at execEmbeddedViewsAction (core.js?593e:14096)
at checkAndUpdateView (core.js?593e:13788)
at callViewAction (core.js?593e:14138)

Can someone help me?

[Tri-vikram]

can u send me the link of global address info ... it would be helpfull to me
https://www.youtube.com/watch?v=EIAgTDyGocI