ColourPicker class

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

User avatar
pickle
Briney Mod
Posts: 6445
Joined: Mon Jan 19, 2004 6:11 pm
Location: 53.01N x 112.48W
Contact:

Post by pickle »

From my extensive background in XML & XSLT that I've learned in the last 10 minutes, I'm not really sure how I could cut down on markup. XSLT is just used to transform an XML file into an HTML file - and we're back to square one.

Am I missing something?



It's just occurred to me that I could move most of the style declarations into classes rather than re-declaring them. I'll probably do that today or tomorrow
Real programmers don't comment their code. If it was hard to write, it should be hard to understand.
User avatar
Chris Corbyn
Breakbeat Nuttzer
Posts: 13098
Joined: Wed Mar 24, 2004 7:57 am
Location: Melbourne, Australia

Post by Chris Corbyn »

pickle wrote:Am I missing something?
Yes. The XSLT stylesheet can be executed on the browser. That means that the browser does't *download* all the markup apart from the XML and XSLT but it then renders a complete XHTML page. XSLT is like a templating lanaguage. You can deal with conditions and loops etc so there's no need to duplicate all that markup so much. If I get time I'll have a poke about for you. I cut down a lot of code from a CSS pie-chart generator that used thousands of tiny little <div> elements to start with.
User avatar
pickle
Briney Mod
Posts: 6445
Joined: Mon Jan 19, 2004 6:11 pm
Location: 53.01N x 112.48W
Contact:

Post by pickle »

Ok....still not sure I get it

Do you think the long load time is from the download? Why would it be so much faster for me in Firefox, then, than it is in IE?

I was under the assumption that the long page load time was due to rendering.
Real programmers don't comment their code. If it was hard to write, it should be hard to understand.
User avatar
Chris Corbyn
Breakbeat Nuttzer
Posts: 13098
Joined: Wed Mar 24, 2004 7:57 am
Location: Melbourne, Australia

Post by Chris Corbyn »

pickle wrote:Ok....still not sure I get it

Do you think the long load time is from the download? Why would it be so much faster for me in Firefox, then, than it is in IE?

I was under the assumption that the long page load time was due to rendering.
The long load time for me here in the UK is due to TCP traffic. Reducing markup size will bring that down. The load time you're referring to in IE is rendering time (*cough* IE sucks ... oops did I say that out loud?) which can't really be brought down.

One big tip I've found from benchmarks in IE vs FF for rendering (*cough* IE sucks ... oops).

Using CSS class names is far faster than inline style or using IDs. <div> tags render much faster than tables too.

*cough* *sucks* *cough*
User avatar
pickle
Briney Mod
Posts: 6445
Joined: Mon Jan 19, 2004 6:11 pm
Location: 53.01N x 112.48W
Contact:

Post by pickle »

Ok, I've moved most of the style declaration out into classes - seems to load faster - cut down the filesize by 14%.
Real programmers don't comment their code. If it was hard to write, it should be hard to understand.
User avatar
Weirdan
Moderator
Posts: 5978
Joined: Mon Nov 03, 2003 6:13 pm
Location: Odessa, Ukraine

Post by Weirdan »

switching to standards mode from quirks might improve speed as well... try it :)
User avatar
Weirdan
Moderator
Posts: 5978
Joined: Mon Nov 03, 2003 6:13 pm
Location: Odessa, Ukraine

Post by Weirdan »

Javascript part could be improved as well: you can attach event listeners to wrapper div instead of every color cell.
User avatar
pickle
Briney Mod
Posts: 6445
Joined: Mon Jan 19, 2004 6:11 pm
Location: 53.01N x 112.48W
Contact:

Post by pickle »

How could I fix the javascript? How can I determine which colour was clicked on / moused over if I have the listeners in the parent div?
Real programmers don't comment their code. If it was hard to write, it should be hard to understand.
User avatar
Weirdan
Moderator
Posts: 5978
Joined: Mon Nov 03, 2003 6:13 pm
Location: Odessa, Ukraine

Post by Weirdan »

How can I determine which colour was clicked on / moused over if I have the listeners in the parent div?
http://developer.mozilla.org/en/docs/DOM:event.target
User avatar
pickle
Briney Mod
Posts: 6445
Joined: Mon Jan 19, 2004 6:11 pm
Location: 53.01N x 112.48W
Contact:

Post by pickle »

Interesting

... goes to fiddle
Real programmers don't comment their code. If it was hard to write, it should be hard to understand.
User avatar
pickle
Briney Mod
Posts: 6445
Joined: Mon Jan 19, 2004 6:11 pm
Location: 53.01N x 112.48W
Contact:

Post by pickle »

Well, to my complete lack of surprise, I've got it working nicely in Firefox, but IE once again craps all over my hopes and dreams.

Turns out IE doesn't support event.target, but rather event.fromElement. Not a big deal. However, if you go to the example again in IE, you'll notice nothing happens when you click on a colour. That's because event.fromElement is returning null.

The div with all the events attached looks like:

Code: Select all

<div style = "font-size:5pt;" onmouseover = 'showColour(event);' onclick = 'setColour(event);'>
The javascript functions it calls are:

Code: Select all

      function showColour(event)
      {
	target = (event.fromElement) ? event.fromElement : event.target;
	if(target)
	{
	  colour = target.style.backgroundColor;
	  document.getElementById('well').style.backgroundColor = target.style.backgroundColor;
	}
      }

      function setColour(event)
      {
	target = (event.fromElement) ? event.fromElement : event.target;

	if(target)//this condition is never satisfied in IE
	{
	  colour = target.style.backgroundColor;
	  properColour = convertToHex(target.style.backgroundColor);
	  window.opener.document.$this->element_id.value = properColour;
	  window.close();
	}
      }
Real programmers don't comment their code. If it was hard to write, it should be hard to understand.
User avatar
Burrito
Spockulator
Posts: 4715
Joined: Wed Feb 04, 2004 8:15 pm
Location: Eden, Utah

Post by Burrito »

try alerting target and see what it is..
User avatar
pickle
Briney Mod
Posts: 6445
Joined: Mon Jan 19, 2004 6:11 pm
Location: 53.01N x 112.48W
Contact:

Post by pickle »

It shows: [object]
Real programmers don't comment their code. If it was hard to write, it should be hard to understand.
User avatar
Burrito
Spockulator
Posts: 4715
Joined: Wed Feb 04, 2004 8:15 pm
Location: Eden, Utah

Post by Burrito »

ok, try alerting one of its properties then...
User avatar
Chris Corbyn
Breakbeat Nuttzer
Posts: 13098
Joined: Wed Mar 24, 2004 7:57 am
Location: Melbourne, Australia

Post by Chris Corbyn »

Burrito wrote:ok, try alerting one of its properties then...
Of which you can find by:

Code: Select all

for (var prop in theObject)
{
    document.write(prop+'<br />');
}
Post Reply