Skip to content
This repository has been archived by the owner on Feb 8, 2023. It is now read-only.

1165-incompatible-browser-validation-and-warning #1348

Merged
merged 14 commits into from
Jul 1, 2020

Conversation

Dmac26
Copy link
Contributor

@Dmac26 Dmac26 commented May 27, 2020

Summary

Addresses Issue # 1165
This PR is an updated version of the original 1165 PR, which needed several styling updates to account for mobile views and to clean up some syntax. I was unable to directly update the original PR as I did not manage my work flow properly and my branch was significantly behind the dev branch.

This pull request is ready to merge when...

  • Feature branch is starts with the issue number
  • Is connected to its original issue via zenhub 👇
  • All tests are passing and meet coverage, linting, and accessibility requirements. And no security vulnerabilities ⚫️(Jenkins)
  • This code has been reviewed by someone other than the original author

@Dmac26
Copy link
Contributor Author

Dmac26 commented Jun 8, 2020

@briandavidson - Hey Brian, just a heads up. I sent you a message detailing multiple questions and problems I'm having with the current PRs en queue. Gratsi sir!

@Dmac26 Dmac26 requested a review from mtlaney June 9, 2020 15:47
Copy link
Member

@briandavidson briandavidson left a comment

Choose a reason for hiding this comment

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

Along with the changes requested in the other comments, please revert the changes to server/package.json and omit both server/package-lock.json and frontend/package-lock.json from the PR -- those shouldn't be necessary for addressing the issue.

frontend/src/app/app.component.ts Outdated Show resolved Hide resolved
frontend/src/sass/components/_header.scss Outdated Show resolved Hide resolved
@Dmac26
Copy link
Contributor Author

Dmac26 commented Jun 17, 2020

@briandavidson @mtlaney - Hey Mike and Brian, this PR should be good for a final review.

frontend/src/app/app.component.ts Show resolved Hide resolved
frontend/src/app/app.component.ts Outdated Show resolved Hide resolved
frontend/src/app/app.component.ts Outdated Show resolved Hide resolved
@Dmac26 Dmac26 requested review from briandavidson and mtlaney and removed request for mtlaney June 23, 2020 21:35
Copy link
Contributor

@mtlaney mtlaney left a comment

Choose a reason for hiding this comment

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

Code looks good. I tested on safari and chrome and it is working as expected. Also tested mobile responsiveness and it looks good there too. Nice work Dylan

@Dmac26 Dmac26 requested a review from mtlaney June 29, 2020 16:12
@Dmac26 Dmac26 requested review from briandavidson and removed request for briandavidson June 29, 2020 16:15
Copy link
Member

@briandavidson briandavidson left a comment

Choose a reason for hiding this comment

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

💥 💥 💥

g2g!

Comment on lines +102 to +115
switch (parsedBrowserInfo[1]) {
case 'OPR': {
this.browserName = 'Opera';
break;
}
case 'Edge': {
this.browserName = 'Edge';
break;
}
default: {
this.browserName = 'notChrome';
break;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This switch is much cleaner, nice work here @Dmac26!

@Dmac26 Dmac26 merged commit fda7b46 into dev Jul 1, 2020
@abdul-fs abdul-fs mentioned this pull request Jul 2, 2020
9 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants