Closed apex-omontgomery closed 6 years ago
Hey! I am new to open source contribution and this will be my first issue. Can i work on this??
Thanks.
@erathinam You sure can - I'll assign myself as a placeholder for you.
Can you submit a wireframe/mockup of the UI you intend to build, so the rest of the team can weigh in and give you the greenlight before you start coding? Anything along the lines of 'notification banner, or browser redirect or something that indicates they need to user to use an updated browse' as @wimo7083 suggests should work.
This is what i have in my mind right now.
Let me know if it needs to be improved.
Thanks.
I was looking for the description from @kylemh he said he wanted a redirect and new page, but I can't find that statement anywhere. I think your design looks fine, just want to make sure your time isn't wasted cause I can't find the requirements.
@erathinam I'd prefer that the entire page be covered in the message. I don't want people to be able to interact with or see our app if they're using an unsupported browser.
@kylemh Ya. im thinking of removing the close button like the below one, so user would not be able to interact with the app. And also, should i just enable the notification bar for ie<=11?
Let me know what you think. Thanks.
@erathinam That's better, but can you not have a transparent background? Simply block out the underlaying content.
Sure. I can do that. It will look like this one. Also, if this is ok, can i go ahead and commit it. So, should i create a new branch with name as "feature/ notify IE11 users they are unsuported". Push my changes to this and create a pull request as mentioned in git workflow. Sorry to ask this question, just need to know since i am new to opensource.
Thanks.
If you think it's ready for a PR, go ahead and do it. I'd be more assertive with the message.
The message you're saying is implying there might be issues, IE 11 is a known issues. It's a conscious decision to not support it due to effort and security. I'm sure the content of your message will get fleshed out in the PR.
The message looks a bit vague to me - ''Please upgrade your browser for a seamless experience" could lead users to continue to try and use IE, just try to upgrade its version somehow (instead of switching browsers, which is what is required in order to view the site correctly).
IMO, we should explicitly tell state that the IE browser is not supported, and list which browsers are supported so a user knows what they can switch to (Chrome, Edge, Safari, Firefox, etc).
Here's an example from Basecamp. We don't necessarily need to go all out and show browser icons / links (thought that would be nice), but we should at least tell the user what browsers they can use:
@jjhampton Thanks for the suggestion. I had that thought too in my mind. Il go ahead and change it. il try to add icons too. Also, i have some small questions about the right way to put my code into repo. Would it be ok to ask those questions in slack channel. How to get invite to the slack channel? Should i sign up at this link? https://operationcode.org/join.
@erathinam Yes, that's the correct link to sign up for Operation Code. Once you submit the form, you should get an email with an invite to the Slack community. The channel to discuss website / repo contributions is #oc-projects
. Look forward to seeing you there!
Unassigned and restrictive label removed. There should be enough discussion above for this issue to get tackled.
Also see https://www.sitepoint.com/internet-explorer-conditional-comments/
i'have been busy with school so couldn't keep it active. @kylemh But the conditional comments are not supported for IE10 and 11. As of now, im trying to find the ie browser(and not edge) by !!document.documentMode(tested it on ie11 that returns true and for edge it returns false). Regarding the design , this is what i have.
I'm happy with that design 👍
@erathinam How's the progress coming on this? If you've started some work but haven't finished yet, feel free to push up a branch and open up an 'in-progress' PR, so someone else can see where you're at and help if necessary.
@jjhampton I have started it long back but was faced with problems when trying to hit any other urls(ex: /jobs) some components on that overlay on notification message. So I'm planning to set up a route and display the message. Il try doing it this week. Il meanwhile do what you said.
When tried to commit, during the precommit phase "npm run image-size" failed. so i had to install "image-size". is this a thing that should be added to package.json. If so, i can update my PR.
@erathinam The Git pre-commit hook checking for image sizes should have been removed months ago: see 86bcce034773dd . So that shouldn't be running anymore.
Can you try updating your local repo by rebasing it on Operation Code's current master
branch, and then try again? If you continue to run into issues, please ping someone in the #oc-projects
Slack channel.
@jjhampton Yes. But i see "npm run image-size" section in package.json in the master branch. So i think this should be removed too.
@erathinam Ah, yeah, that's somehow related to the linter, looks like. Feel free to go ahead and remove the "*.{jpg,jpeg,png,gif,svg}": "npm run image-size"
from package.json for now.
Whenever you open your PR, we'll go ahead and upload the image to our AWS S3 anyway, and remove it from the GitHub repo.
Cool. PR opened.
Feature
Why is this feature being added?
Some way of notifying users that IE11 is unsuported, so we don't have to answer this question again.
What should your feature do?
Convey to users that we understand the view doesn't look right, and they should use a supported browser.
notification banner, or browser redirect or something that indicates they need to user to use an updated browser.