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

Refactor & Update Progress Bars #379

Merged
merged 5 commits into from
Feb 13, 2013
Merged

Refactor & Update Progress Bars #379

merged 5 commits into from
Feb 13, 2013

Conversation

caycefischer
Copy link
Contributor

(trying this again)

Got rid of a lot of markup, replaced with the HTML5 element, which has a cool javascript API and is generally rad. Polyfilled for older browsers too, so should work everywhere IE8+, and on Android/iOS.

Having no iOS device myself, we'll need to test. But performs smashingly on my Nexus.

@caycefischer
Copy link
Contributor Author

re: 8c9cd3c

I tried git stashing this to keep the pull request clean, but actually some re-working the header layout was needed to get the bars working right.

@lefnire
Copy link
Contributor

lefnire commented Feb 12, 2013

Awesome! You're totally right about the gradient, that was pointless. Glad
to see the refactoring on the avatar, that'll clean things up a lot.

I'm a bit concerned about the progress bars, because that introduces new
code we need to maintain. Do the Bootstrap bars not use +
pollyfill? If that's the case, is there a benefit in using said over
Bootstrap's implementation?

On Tuesday, February 12, 2013, Zachary Kain wrote:

re: 8c9cd3chttps://github.com/lefnire/habitrpg/commit/8c9cd3cb67d1b266537716e1e0255da5d2df0cbf

I tried git stashing this to keep the pull request clean, but actually
some re-working the header layout was needed to get the bars working right.


Reply to this email directly or view it on GitHubhttps://github.com//pull/379#issuecomment-13447503.

@caycefischer
Copy link
Contributor Author

I checked it out, Bootstrap just uses <div> markup with inline styles. Arguably this way is less work, because we would only have to maintain the css, as the html is just a single element. This is more future-forward, too (though that isn't incredibly important, divs aren't going anywhere)

@caycefischer
Copy link
Contributor Author

And, I imagine one day in the future we won't be using any bootstrap code anymore, and we can cut the 120kb of css that it weighs

@lefnire
Copy link
Contributor

lefnire commented Feb 12, 2013

Fair enough - I'll check it out soon, and thanks again for the PR!

@lefnire
Copy link
Contributor

lefnire commented Feb 13, 2013

Tested locally. Beautiful. As always, friend Zack, you are awesome.

@lefnire lefnire closed this Feb 13, 2013
lefnire added a commit that referenced this pull request Feb 13, 2013
Refactor & Update Progress Bars
@lefnire lefnire merged commit 917f2f1 into HabitRPG:master Feb 13, 2013
@lefnire
Copy link
Contributor

lefnire commented Feb 13, 2013

Zack FTW

@caycefischer
Copy link
Contributor Author

Suh-weet!

lemondance

@lefnire
Copy link
Contributor

lefnire commented Feb 13, 2013

heads up - getting some funkyness on my android, and iphone: screenshot

@caycefischer
Copy link
Contributor Author

On it. I removed a few lines because they didn't seem to do anything on desktop... seems that I wrote them for mobile in the first place.

@lefnire
Copy link
Contributor

lefnire commented Feb 13, 2013

Don't stress too hard either, next push will be a bit down the pike. Thanks again for all your help

@caycefischer
Copy link
Contributor Author

No problem, I'll figure it out. Unfortunately, I don't have an iOS device to test with, just my Galaxy Nexus. Try just adding display: inline-block to progress in index.styl (line 85), I have a hunch that'll do it. If not, I'll dive into in in a couple days.

@lefnire
Copy link
Contributor

lefnire commented Feb 13, 2013

Closer, no fills: screenshot. I just use xcode emulator for iphone on my laptop, and my android 2.2 (could use eclipse android emulator)

@lefnire
Copy link
Contributor

lefnire commented Feb 13, 2013

got a big derby-auth fix in the pipe, might be pushing soon. I'll take a stab at the missing fills, just chiming in in case you have a top-of-the-dome recommendation

@caycefischer
Copy link
Contributor Author

Top of the dome? Add the rule -webkit-appearance: none to that same selector (line 85).

I gotta get Xcode, I'm running just the Command Line Tools but not having that emulator has been a thorn in my side for a while.

PS: Found a rendering bug in Safari, fix is to remove the border-radius (lines 91-92) Screen Shot 2013-02-13 at 1 11 28 PM

@lefnire
Copy link
Contributor

lefnire commented Feb 13, 2013

Ok, couldn't get it working on mobile even with those 3 things you mentioned. I reverted all these commits in 064abc9 (also see ec2ec3e). When you sit down to it again, just revert my revert.

I think for now, unless you're thinking "oh it's an obvious bug!", we should go back to the original progress-bars, just so we have less to maintain in the short term. Keep them on a separate branch or some such. I wanna get all your avatar box code back in though

@caycefischer
Copy link
Contributor Author

OK, cool. You do your thing, I'll work out all the kinks with the progress
bars in a feature branch and merge it when it's ready.

I'm not very good at using Git for big projects/with others, sorry for the
whole mess.

On Wed, Feb 13, 2013 at 3:34 PM, Tyler Renelle notifications@github.comwrote:

Ok, couldn't get it working on mobile even with those 3 things you
mentioned. I reverted all these commits in 064abc9064abc959b0a72cdc07be6d4b868be1acf93fe6f(also see
ec2ec3ehttps://github.com/lefnire/habitrpg/commit/ec2ec3e87b3e07f5f6fccf7390c208725b6781a9).
When you sit down to it again, just revert my revert.

I think for now, unless you're thinking "oh it's an obvious bug!", we
should go back to the original progress-bars, just so we have less to
maintain in the short term. Keep them on a separate branch or some such. I
wanna get all your avatar box code back in though


Reply to this email directly or view it on GitHubhttps://github.com//pull/379#issuecomment-13516645.

– Zachary Kain

Designer, Creative Technologist
416-712-8895
zakkain@gmail.com
→ view my resume/profile http://zerply.com/zakkain/public

@lefnire
Copy link
Contributor

lefnire commented Feb 13, 2013

Not at all, you were fine - I was the one that merged into master before
testing it fully, I think we're all kinda learning together here :)

On Wednesday, February 13, 2013, Zachary Kain wrote:

OK, cool. You do your thing, I'll work out all the kinks with the progress
bars in a feature branch and merge it when it's ready.

I'm not very good at using Git for big projects/with others, sorry for the
whole mess.

On Wed, Feb 13, 2013 at 3:34 PM, Tyler Renelle <notifications@github.com<javascript:_e({}, 'cvml', 'notifications@github.com');>>wrote:

Ok, couldn't get it working on mobile even with those 3 things you
mentioned. I reverted all these commits in 064abc9<
064abc959b0a72cdc07be6d4b868be1acf93fe6f>(also
see
ec2ec3e<
ec2ec3e87b3e07f5f6fccf7390c208725b6781a9>).

When you sit down to it again, just revert my revert.

I think for now, unless you're thinking "oh it's an obvious bug!", we
should go back to the original progress-bars, just so we have less to
maintain in the short term. Keep them on a separate branch or some such.
I
wanna get all your avatar box code back in though


Reply to this email directly or view it on GitHub<
https://github.com/lefnire/habitrpg/pull/379#issuecomment-13516645>.

– Zachary Kain

Designer, Creative Technologist
416-712-8895
zakkain@gmail.com <javascript:_e({}, 'cvml', 'zakkain@gmail.com');>
→ view my resume/profile http://zerply.com/zakkain/public


Reply to this email directly or view it on GitHubhttps://github.com//pull/379#issuecomment-13516794.

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

Successfully merging this pull request may close these issues.

2 participants