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

Update outdatedbrowser.js #232

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

schonarth
Copy link

Tolerate absence of btnClose and btnUpdate, when using custom content.

Tolerate absence of btnClose and btnUpdate, when using custom content.
@alexandre-works
Copy link
Collaborator

@schonarth thanks 👍
But why do you want to use the plugin without the option to have the button to update?
Cheers

@schonarth
Copy link
Author

My client is very specific in how they want the message bar to appear,
especially the content. They don't want the prominent button, but rather a
link in the text, which is what I did and this small change allowed.

While I was at it, I also covered the same condition for the Close button,
although this is not my case right now.

Thanks for considering my small contribution =]

Cheers
Gus

Em ter, 26 de abr de 2016 às 05:54, Alexandre Gomes <
notifications@github.com> escreveu:

@schonarth https://github.com/schonarth thanks 👍
But why do you want to use the plugin without the option to have the
button to update?
Cheers


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#232 (comment)

@alexandre-works
Copy link
Collaborator

@schonarth Thank you 👍
I am giving you my personal opinion: why add an extra if in the js, if we can do that with css? I know it's just an extra if, and if we keep the buttons we have extra html to load, but even so I prefer the CSS way:

.btn{
 position: absolute;
 left: -5555px;
 top: -5555px;
}

We know that js is more prone to errors then css, and this way I know I don't need to test for older browsers (IE6, etc) because I am really sure it will work.

Personally I would do this approach, so maybe we will wait for the merge if more ppl wants your approach.
Thanks

@schonarth
Copy link
Author

It works, though it changes the base logic. My point was to make the change as small as possible, just to avoid exceptions in my particular case.

By all means, do as you prefer =]

@alexandre-works
Copy link
Collaborator

@schonarth let see what is the feedback of the community.
Once again thank you man 👍

@brunoamorim
Copy link
Collaborator

@alexandre-works do we received any feedback from the community about this subject?

@alexandre-works
Copy link
Collaborator

@brunoamorim no. We had no suggestions or requests regarding this feature. Still thing that the CSS approach is the best solution for this.

@schonarth
Copy link
Author

I just think that a null check is never too much for a more robust piece of
software. I did it on my copy. I think others might benefit from it too.

Vc q sabe.

Em ter, 11 de out de 2016 às 17:29, Alexandre Gomes <
notifications@github.com> escreveu:

@brunoamorim https://github.com/brunoamorim no. We had no suggestions
or requests regarding this feature. Still thing that the CSS approach is
the best solution for this.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#232 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAMhyx_hUsZJoGoT86TvY4YVWlM6qCWmks5qy_G5gaJpZM4IPbJ0
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants