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.

Sign up for GitHub

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

Jump to bottom

fix(no-ie): add js snipped to check for internet explorer (#445) #519

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

fix(no-ie): add js snipped to check for internet explorer (#445) #519

wants to merge 8 commits into from

Conversation

VanLampe
Copy link
Contributor

@VanLampe VanLampe commented Mar 10, 2020 •
edited by mschwrdtnr
Loading

resolves #445
check if internet explorer is used and offer links to more modern/common browsers

@VanLampe
fix(no-ie): add js snipped to check for internet explorer (#445)
5dc9f7c 
check for ie
links to open pb in other common browsers
@VanLampe
Copy link
Contributor Author

Sorry that it took so long, but I did not found time to start working at this. Please just tell me, what you guys think of the solution. I did not spend much time in css styling.

@mmsgithub-ci
Copy link
Collaborator

Preview Environment ready at https://pr-519.demo-phonebook.me

@DanielHabenicht
Copy link
Collaborator

DanielHabenicht commented Mar 10, 2020 •
edited
Loading

Bis auf die zwei Kommentare sieht es sehr gut aus. Kein Problem das es länger gedauert hat.

@VanLampe
Copy link
Contributor Author

@DanielHabenicht thanks for your review! Does the calls to open the application with chrome, firefox and opera work for you? If I try that in the demo-environment it does not.

@DanielHabenicht
Copy link
Collaborator

DanielHabenicht commented Mar 10, 2020 •
edited
Loading

Nope but, working on my comment should fix it.

@VanLampe
Copy link
Contributor Author

VanLampe commented Mar 10, 2020 •
edited
Loading

Nope but, my working on my comment should fix it.

I had to change security settings and enable ActiveX..now IE asks if I want to execute firefox for example.
image

I could not find a way to open the app in firefox, chrome or opera without ActiveX.
Edge should work finde because I can call it via href..

@VanLampe @DanielHabenicht
fix(no-ie): resolve comment
f12f03a 
Co-Authored-By: DanielHabenicht <[email protected]>
@mmsgithub-ci
Copy link
Collaborator

Preview Environment ready at https://pr-519.demo-phonebook.me

@DanielHabenicht
Copy link
Collaborator

I think we shouldn't use ActiveX in this context. Just provide a link where users can download the other browser. They can then switch it manually.

@VanLampe
Copy link
Contributor Author

I think we shouldn't use ActiveX in this context. Just provide a link where users can download the other browser. They can then switch it manually.

I also think, ActiveX isnt a good solution. I will revoke those changes. But I will keep the Link to open with edge, because this works without ActiveX.

@VanLampe
fix(no-ie): remove AxtiveX elements
6227bd7 
add download links for modern browsers
@VanLampe
Copy link
Contributor Author

@DanielHabenicht I updated the code, please let me know what you think.

@mmsgithub-ci
Copy link
Collaborator

Preview Environment ready at https://pr-519.demo-phonebook.me

@VanLampe VanLampe requested a review from paule96 March 11, 2020 09:25
mschwrdtnr
mschwrdtnr previously approved these changes Mar 11, 2020
View reviewed changes
Copy link
Member

@mschwrdtnr mschwrdtnr left a comment

Choose a reason for hiding this comment

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

Finde es sehr gut, dass man mit einem Klick direkt zu Edge gelangt. Spart dem Nutzer nochmal 10 Sekunden fürs öffnen eines Browsers und öffnen des Phonebooks (y)

@mschwrdtnr mschwrdtnr added the bug Something isn't working label Mar 11, 2020
@VanLampe VanLampe dismissed stale reviews from DanielHabenicht and mschwrdtnr via c002afb March 11, 2020 12:38
VanLampe and others added 2 commits March 11, 2020 13:39
@VanLampe @DanielHabenicht
fix(no-ie):resolve comment
4e65df1 
Co-Authored-By: DanielHabenicht <[email protected]>
@VanLampe @DanielHabenicht
fix(no-ie): resolve comment
ce2136c 
Co-Authored-By: DanielHabenicht <[email protected]>
DanielHabenicht
DanielHabenicht reviewed Mar 11, 2020
View reviewed changes
Phonebook.Frontend/src/styles.scss
Comment on lines +185 to +187
top: 38%;
left: 38%;
position: absolute;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
top: 38%;
left: 38%;
position: absolute;
margin: auto;
height: 200px;

Copy link
Collaborator

Choose a reason for hiding this comment

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

damit ist es auch genau in der Mitte

DanielHabenicht
DanielHabenicht reviewed Mar 11, 2020
View reviewed changes
Phonebook.Frontend/src/styles.scss
position: absolute;
}
.pb-no-ie a {
cursor: pointer;
Copy link
Collaborator

Choose a reason for hiding this comment

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

das ist eigentlich überfüssig, <a> sind immer mit pointer belegt :)

@DanielHabenicht
Copy link
Collaborator

Wie machen wir das eigentlich mit der Übersetzung? Kannst du dafür eine Issue aufmachen?

@mmsgithub-ci
Copy link
Collaborator

Preview Environment ready at https://pr-519.demo-phonebook.me

@mmsgithub-ci
Copy link
Collaborator

Preview Environment ready at https://pr-519.demo-phonebook.me

@VanLampe
Copy link
Contributor Author

Wie machen wir das eigentlich mit der Übersetzung? Kannst du dafür eine Issue aufmachen?

Meinst du denn, dass man dafür wirklich eine Übersetzung brauchst? Ich hatte das auch erst überlegt, aber fand dann den Aufwand etwas zu hoch, da die Message ja eigentlich recht klar ist..

@DanielHabenicht
Copy link
Collaborator

Wie machen wir das eigentlich mit der Übersetzung? Kannst du dafür eine Issue aufmachen?

Meinst du denn, dass man dafür wirklich eine Übersetzung brauchst? Ich hatte das auch erst überlegt, aber fand dann den Aufwand etwas zu hoch, da die Message ja eigentlich recht klar ist..

dann vielleicht nur auf englisch?

@DanielHabenicht
Copy link
Collaborator

Kannst du außerdem die Komponenten löschen die dadurch nun überflüssig geworden sind?
IEWarningDialog etc.

paule96
paule96 approved these changes Apr 8, 2020
View reviewed changes
Phonebook.Frontend/src/index.html Show resolved Hide resolved
@mschwrdtnr
Copy link
Member

Hey @FrankLambrette how is it going? If I am right you only need to provide an English text and don't need to translate it :)

@DanielHabenicht
Copy link
Collaborator

And remove old is deps

@mschwrdtnr
Copy link
Member

mschwrdtnr commented Aug 5, 2020 •
edited
Loading

Stuff to resolve this PR🎉 🎉

  • Remove IEWarningDialog (Angular Dialog not index.html)
  • Resolve Style Issues (Comments)
  • Provide only English Dialog Text

@DanielHabenicht DanielHabenicht mentioned this pull request Aug 5, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanielHabenicht DanielHabenicht DanielHabenicht left review comments

@paule96 paule96 paule96 approved these changes

@mschwrdtnr mschwrdtnr mschwrdtnr left review comments

@Zeroks77 Zeroks77 Awaiting requested review from Zeroks77

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
bug Something isn't working hackthon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

No info dialog for IE users
5 participants
@VanLampe @mmsgithub-ci @DanielHabenicht @mschwrdtnr @paule96