Development
Threads by month
- ----- 2026 -----
- June
- May
- April
- March
- February
- January
- ----- 2025 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2024 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2023 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2022 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2021 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2020 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2019 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2018 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2017 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2016 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2015 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2014 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2013 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2012 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2011 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2010 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2009 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2008 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2007 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2006 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2005 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
September 2005
- 78 participants
- 615 discussions
Issue status update for
http://drupal.org/node/27140
Post a follow up:
http://drupal.org/project/comments/add/27140
Project: Drupal
Version: cvs
Component: contact.module
Category: bug reports
Priority: normal
Assigned to: Souvent22
Reported by: m3avrck
Updated by: Souvent22
-Status: patch (code needs review)
+Status: patch (ready to be committed)
Attachment: http://drupal.org/files/issues/contact_full_2.patch (6.35 KB)
patch re-roll due to head change.
Souvent22
Previous comments:
------------------------------------------------------------------------
Wed, 20 Jul 2005 14:57:03 +0000 : m3avrck
Using the site-wide contact forms, any subjects with an '&' cannot be
deleted. Clicking delete for these subjects brings up the expected
action flow, however, confirming the delete does nothing. Subject still
exists.
------------------------------------------------------------------------
Mon, 25 Jul 2005 13:45:38 +0000 : m3avrck
It appears this issue might be linked with this one:
http://drupal.org/node/23685
A problem with mod_rewrite and not the actual contact module.
------------------------------------------------------------------------
Wed, 31 Aug 2005 16:51:56 +0000 : Souvent22
Attachment: http://drupal.org/files/issues/contact.module_6.patch (4.7 KB)
Redid how contacts work. It now references contacts by ID instead of
their category as the unique identifier. It also allows for contacts to
have weights, so that one may control where the contact will appear in
the listing.
------------------------------------------------------------------------
Wed, 31 Aug 2005 16:53:01 +0000 : Souvent22
Attachment: http://drupal.org/files/issues/updates.inc (27.23 KB)
This is the update.inc for the database cahnges that goes along with
this patch.
------------------------------------------------------------------------
Wed, 31 Aug 2005 18:31:37 +0000 : Souvent22
Attachment: http://drupal.org/files/issues/contact.module_7.patch (4.5 KB)
Updated patch.
------------------------------------------------------------------------
Wed, 31 Aug 2005 18:31:55 +0000 : Souvent22
Attachment: http://drupal.org/files/issues/updates.inc_4.patch (1.29 KB)
Updated database patch.
------------------------------------------------------------------------
Sat, 03 Sep 2005 16:17:21 +0000 : Cvbge
Hi,
0. You should keep all changes in one file, i.e. pake one patch, not
patch for every file
1. You should include changes for database/database.{mysql,pgsql}
2. IIRC auto_increment was depreciated, you should use db_next_id()
(but I see auto_increment in cvs...)
3. You use "WHERE cid = '%s'", but cid is integer so this should
probably be %d (although this probably would not break anything..)
------------------------------------------------------------------------
Wed, 07 Sep 2005 17:31:21 +0000 : Souvent22
Attachment: http://drupal.org/files/issues/contact_full.patch (6.38 KB)
Ok, I've taken your suggestions in, and i think I have it this time. Let
me know if this is a correct format for a patch. This only fixes the bug
of not being able to delete special characters.
------------------------------------------------------------------------
Wed, 07 Sep 2005 17:37:30 +0000 : m3avrck
Check your database updates, the UNIQUE fields don't look right
syntactically.
------------------------------------------------------------------------
Wed, 07 Sep 2005 18:23:07 +0000 : Souvent22
Attachment: http://drupal.org/files/issues/contact_full_0.patch (6.25 KB)
Ok, i apologize for the last upload. Pleasle ignore. I send the wrong
file. :-[. Anyway, here's a tested, tried, and correct patch.
------------------------------------------------------------------------
Wed, 07 Sep 2005 18:26:33 +0000 : m3avrck
+1 patch works great! Latest HEAD version applied with update.php works
great, modifies database correctly for MySQL (and PostgreSQL looks
correct as well) and contacts can now be deleted if they have '&' in
the subject. Also, dumped entire database and created from modified
database.mysql and everything works great. I'd say this patch is ready
to be committed!
------------------------------------------------------------------------
Wed, 07 Sep 2005 21:44:47 +0000 : Cvbge
Hi, patch does not applies to currect cvs anymore.
For the database.pgsql you need to remove (11) from int(11) and also
first "category" from UNIQUE line, so it looks like this:
CREATE TABLE contact (
cid int NOT NULL,
category varchar(255) NOT NULL default '',
recipients text NOT NULL default '',
reply text NOT NULL default '',
UNIQUE (category),
PRIMARY KEY (cid)
);
The updates are also not compatibile with postgres, but postgres
upgrade path is completly broken in cvs and I'll fix it later, so do
not pay attention to it.
------------------------------------------------------------------------
Thu, 08 Sep 2005 13:05:54 +0000 : Souvent22
Hello. Thanks for the feedback. I'm in the middle of fixing it, but i
have a question. You'll have to excuse my ignorance with PostGres, I'm
not as familiar with it as I am MySQL. With that said, I have a
question.....
I am trying to make my update where one does not lose data. However, in
PostGres, I have to make a seperate "temp" table before hand and then
copy the date over the the new table (cause i don't know the exact name
of the constraint to drop e.g. the primary key).
Basically I'm asking if there is a better suggestion in which I doint
have to use a temp table, and i could just manipulate the original
table? Thanks.
------------------------------------------------------------------------
Thu, 08 Sep 2005 13:12:35 +0000 : Souvent22
Also,
I created a temp name for my temp table. However, for prefix purposes,
should I enclose it? Example:
$temp_name = rand();
$res = db_query("SELECT * FROM $temp_name");
or
$temp_name = rand();
$res = db_query("SELECT * FROM {$temp_name}");
Thanks.
------------------------------------------------------------------------
Thu, 08 Sep 2005 13:55:40 +0000 : Souvent22
Clarification:
$temp_name = rand();
db_query("CREATE TABLE $temp_name AS SELECT * FROM {contact}");
$res = db_query("SELECT * FROM $temp_name");
or
$temp_name = rand();
db_query("CREATE TABLE {$temp_name} AS SELECT * FROM {contact}");
$res = db_query("SELECT * FROM {$temp_name}");
------------------------------------------------------------------------
Thu, 08 Sep 2005 17:52:06 +0000 : Cvbge
"cause i don't know the exact name of the constraint to drop e.g. the
primary key
"
Yes, you know it, it's tablename_pkey. In this case it's contact_pkey.
You can drop it by dropping index with it's name (you could drop
constraint with it's name but it was not available in 7.2)
Adding a column and adding unique attribute is also doable without temp
table. You can look at (some) previous updates. But some of them are
incorrect ;)
By saying that I'll fix it later I meant that I want to introduce
function to modify tables and don't want to make temporary fixes now.
------------------------------------------------------------------------
Thu, 08 Sep 2005 19:47:08 +0000 : Souvent22
Thanks. yes, you're right. didn't know if that was universal across the
board though since that'st he default, but someone could give their
primary key constraint a diff. name. But, with your comment, I'll wait
and let you take care of fixing my pgsql portion of my patch. Thanks for
your comments though, really helpful...and not confusing. :).
------------------------------------------------------------------------
Fri, 09 Sep 2005 19:16:19 +0000 : Souvent22
Attachment: http://drupal.org/files/issues/contact_full_1.patch (6.23 KB)
Got the postgres correct this time. No temp table needed.
1
0
[drupal-devel] [feature] Prevent accidentally navigating away from pages where content has changed
by m3avrck 13 Sep '05
by m3avrck 13 Sep '05
13 Sep '05
Issue status update for
http://drupal.org/node/30220
Post a follow up:
http://drupal.org/project/comments/add/30220
Project: Drupal
Version: cvs
Component: base system
Category: feature requests
Priority: normal
Assigned to: Anonymous
Reported by: m3avrck
Updated by: m3avrck
Status: patch (code needs review)
Attachment: http://drupal.org/files/issues/formcheck_3.js (1.64 KB)
Updated JS styling and JS-killswitch after talking with Unconed on IRC.
m3avrck
Previous comments:
------------------------------------------------------------------------
Thu, 01 Sep 2005 22:47:42 +0000 : m3avrck
How many times have you been posting a comment or working on a new post
when you click on a link in another application and you navigate away
from the content you are editing, losing all of your changes?
Well I say we introduce a script, much similiar to that of Blogger,
that uses the Javascript window.onbeforeunload handler to prevent this
from happening. We can set this handler to call a function that
compares the contents of all forms when the page loads and then quickly
compares that to the current contents just before the user is about to
leave the page. If they have changed, we should prompt the user so they
don't lose their work. This would save many headaches and degrade nicely
for users without Javascript.
Additionally, this script should work with the newly introduced
JS-based upload feature to prevent navigating away as a file is being
uploaded as well.
If I have sometime I'll start work on a patch tomorrow, just wanted to
get the idea into the queue right now :)
------------------------------------------------------------------------
Sat, 10 Sep 2005 23:48:33 +0000 : StuartDH
Sounds like a great feature to me. We regularly get authors and editors
telling us that they've lost the last hour of work by accidentally
navigating away, and I've even just had one of them email me about it a
few minutes ago, so it'd be great if you could put something together
for this.
Cheers
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:33:58 +0000 : MichelleC
Sounds great to me, too. I often have several tabs open doing stuff and
it's easy to lose my place and leave a page with unfinished edits.
Would this alert you when you're going to close an unsaved tab/window
as well?
Michelle
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:34:19 +0000 : MrMattles
Great for us multitaskers that rush and click the wrong thing
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:36:52 +0000 : chx
Very strong -1 as this is IE only.
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:38:40 +0000 : praseodymium
Konqueror already does this, +1 for the idea in general.
------------------------------------------------------------------------
Mon, 12 Sep 2005 15:07:21 +0000 : webchick
Yeah, I think chx is correc that onbeforeunload is an extension
developed by MS [1] (it's not in the ECMAScript specification [2].
However, I found out that this is implemented in Mozilla since 1.7 [3]
(which corresponds to Firefox 1.0, I think?) and I've tested
Blogger.com's version of this functionality in:
Firefox 1.0.6 (working)
Firefox Deer Park Alpha 1 (working)
Opera 8.0.2 (working)
Safari 1.3 (does NOT work)
Safari 2.0.1 (working)
So it seems to be a 'de facto' standard if nothing else.
[1]
http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onbeforeun…
[2]
http://www.ecma-international.org/publications/standards/Ecma-262.htm
[3] http://www.mozilla.org/status/2004-03-01.html
------------------------------------------------------------------------
Mon, 12 Sep 2005 15:09:15 +0000 : webchick
Also, I should clarify... that "does NOT work" in Safari doesn't display
any errors or anything, it just doesn't save the form results as
expected (so pre-Tiger Safari users are going to be used to this). So I
don't see any harm in including this functionality since it seems to
degrade gracefully in non-supporting browsers.
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:10:18 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/node.module_11.patch (951 bytes)
And a patch is ready! Code was based on many example, including this [4]
and this [5] along with adding my own thoughts and what not ;)
[4]
http://www.codestore.org/store.nsf/cmnts/451FA051398A9AF486256EE0000FB7D1?O…
[5] http://www.blogger.com/app/scripts/formcheck.js
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:10:45 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck.js (1.54 KB)
And here is the JS to throw in misc/.
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:22:56 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_0.js (1.61 KB)
Better JS file attached without tabs. Also, tested and working great in
FF and IE6. Doesn't work in Opera 8, however no errors are present, I
believe this is just because the event handler is not supported. If
there is a work around for Opera, please let me know!
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:24:18 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_1.js (1.61 KB)
One more try with those tabs.
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:32:48 +0000 : moshe weitzman
hmmm. i usually find these prompts annoying. any chance we can attach
the behavior only to the node and comment forms? those are the "high
risk" areas. what do others think?
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:42:39 +0000 : m3avrck
Yes, the JS is only loaded on the node edit/create pages, won't affect
any admin/login/etc form. Also, the prompt is unobstrusive and if you
goto save a node you don't get a warning that the contents have changed
because you are explicity saving the node (hence no reason!).
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:53:01 +0000 : Dries
I want some JS-folks to review the code.
------------------------------------------------------------------------
Mon, 12 Sep 2005 22:09:10 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/drupal_15.patch (1.94 KB)
After talking with Drumm on IRC, this patch makes the integration with
Drupal much simpler and it's off by default. Only turned on for the
node edit/change page right now but any module/form can easily add this
by passing a 'TRUE' parameter.
------------------------------------------------------------------------
Mon, 12 Sep 2005 23:38:35 +0000 : nedjo
+1 on idea, js looks generally good, a few suggestions:
1. Instead of writing onsubmit via PHP, use a js addLoadEvent call.
2. Shouldn't the return false in isElementChanged be at the end
(outside the switch block)?
3. It's be nice to find a way to make messages like 'You have unsaved
changes.' translatable. Pass in global js variables via a t('') call?
4. onbeforeunload event should probably be in a if isJsEnabled test,
and should parallel drupal.js's event adding (see addLoadEvent).
5. Comments should be in standard format and in present tense, e.g.,
/**
* Checks to see if a form has been changed after the page loads
*/
6. e_ should be just e to match other js files, e.g., autocomplete.
------------------------------------------------------------------------
Tue, 13 Sep 2005 13:54:35 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_2.js (1.58 KB)
Ok here is an updated JS file.
A few notes, I couldn't get the addSubmitEvent() in drupa.js to
reliably work so I had to set the isSubmit() true in the PHP creation
of the form. If you look at Blogger.com, they do this as well... so
either we both missed an obvious way to do this, or that is the most
practical. Hopefully some wise JS gurus will chime in with an answer.
Same goes for onbeforeunload event, which is completely different then
addEvent defined in drupal.js.
As for the t('') I agree this would be useful but I'm not sure of the
best way to do this. One way would be to put in form() a t('') passed
in, and then write this to a var in formcheck.php which returns a JS
file type. Thoughts?
------------------------------------------------------------------------
Tue, 13 Sep 2005 14:06:44 +0000 : Junyor
FWIW, Opera doesn't support the onbeforeunload event. However, this
issue doesn't affect Opera anyway: form contents are retained in
history as long as the page isn't closed. IOW, this feature doesn't
work in Opera, but it isn't needed either.
1
0
[drupal-devel] [feature] Prevent accidentally navigating away from pages where content has changed
by Junyor 13 Sep '05
by Junyor 13 Sep '05
13 Sep '05
Issue status update for
http://drupal.org/node/30220
Post a follow up:
http://drupal.org/project/comments/add/30220
Project: Drupal
Version: cvs
Component: base system
Category: feature requests
Priority: normal
Assigned to: Anonymous
Reported by: m3avrck
Updated by: Junyor
Status: patch (code needs review)
FWIW, Opera doesn't support the onbeforeunload event. However, this
issue doesn't affect Opera anyway: form contents are retained in
history as long as the page isn't closed. IOW, this feature doesn't
work in Opera, but it isn't needed either.
Junyor
Previous comments:
------------------------------------------------------------------------
Thu, 01 Sep 2005 22:47:42 +0000 : m3avrck
How many times have you been posting a comment or working on a new post
when you click on a link in another application and you navigate away
from the content you are editing, losing all of your changes?
Well I say we introduce a script, much similiar to that of Blogger,
that uses the Javascript window.onbeforeunload handler to prevent this
from happening. We can set this handler to call a function that
compares the contents of all forms when the page loads and then quickly
compares that to the current contents just before the user is about to
leave the page. If they have changed, we should prompt the user so they
don't lose their work. This would save many headaches and degrade nicely
for users without Javascript.
Additionally, this script should work with the newly introduced
JS-based upload feature to prevent navigating away as a file is being
uploaded as well.
If I have sometime I'll start work on a patch tomorrow, just wanted to
get the idea into the queue right now :)
------------------------------------------------------------------------
Sat, 10 Sep 2005 23:48:33 +0000 : StuartDH
Sounds like a great feature to me. We regularly get authors and editors
telling us that they've lost the last hour of work by accidentally
navigating away, and I've even just had one of them email me about it a
few minutes ago, so it'd be great if you could put something together
for this.
Cheers
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:33:58 +0000 : MichelleC
Sounds great to me, too. I often have several tabs open doing stuff and
it's easy to lose my place and leave a page with unfinished edits.
Would this alert you when you're going to close an unsaved tab/window
as well?
Michelle
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:34:19 +0000 : MrMattles
Great for us multitaskers that rush and click the wrong thing
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:36:52 +0000 : chx
Very strong -1 as this is IE only.
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:38:40 +0000 : praseodymium
Konqueror already does this, +1 for the idea in general.
------------------------------------------------------------------------
Mon, 12 Sep 2005 15:07:21 +0000 : webchick
Yeah, I think chx is correc that onbeforeunload is an extension
developed by MS [1] (it's not in the ECMAScript specification [2].
However, I found out that this is implemented in Mozilla since 1.7 [3]
(which corresponds to Firefox 1.0, I think?) and I've tested
Blogger.com's version of this functionality in:
Firefox 1.0.6 (working)
Firefox Deer Park Alpha 1 (working)
Opera 8.0.2 (working)
Safari 1.3 (does NOT work)
Safari 2.0.1 (working)
So it seems to be a 'de facto' standard if nothing else.
[1]
http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onbeforeun…
[2]
http://www.ecma-international.org/publications/standards/Ecma-262.htm
[3] http://www.mozilla.org/status/2004-03-01.html
------------------------------------------------------------------------
Mon, 12 Sep 2005 15:09:15 +0000 : webchick
Also, I should clarify... that "does NOT work" in Safari doesn't display
any errors or anything, it just doesn't save the form results as
expected (so pre-Tiger Safari users are going to be used to this). So I
don't see any harm in including this functionality since it seems to
degrade gracefully in non-supporting browsers.
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:10:18 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/node.module_11.patch (951 bytes)
And a patch is ready! Code was based on many example, including this [4]
and this [5] along with adding my own thoughts and what not ;)
[4]
http://www.codestore.org/store.nsf/cmnts/451FA051398A9AF486256EE0000FB7D1?O…
[5] http://www.blogger.com/app/scripts/formcheck.js
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:10:45 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck.js (1.54 KB)
And here is the JS to throw in misc/.
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:22:56 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_0.js (1.61 KB)
Better JS file attached without tabs. Also, tested and working great in
FF and IE6. Doesn't work in Opera 8, however no errors are present, I
believe this is just because the event handler is not supported. If
there is a work around for Opera, please let me know!
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:24:18 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_1.js (1.61 KB)
One more try with those tabs.
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:32:48 +0000 : moshe weitzman
hmmm. i usually find these prompts annoying. any chance we can attach
the behavior only to the node and comment forms? those are the "high
risk" areas. what do others think?
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:42:39 +0000 : m3avrck
Yes, the JS is only loaded on the node edit/create pages, won't affect
any admin/login/etc form. Also, the prompt is unobstrusive and if you
goto save a node you don't get a warning that the contents have changed
because you are explicity saving the node (hence no reason!).
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:53:01 +0000 : Dries
I want some JS-folks to review the code.
------------------------------------------------------------------------
Mon, 12 Sep 2005 22:09:10 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/drupal_15.patch (1.94 KB)
After talking with Drumm on IRC, this patch makes the integration with
Drupal much simpler and it's off by default. Only turned on for the
node edit/change page right now but any module/form can easily add this
by passing a 'TRUE' parameter.
------------------------------------------------------------------------
Mon, 12 Sep 2005 23:38:35 +0000 : nedjo
+1 on idea, js looks generally good, a few suggestions:
1. Instead of writing onsubmit via PHP, use a js addLoadEvent call.
2. Shouldn't the return false in isElementChanged be at the end
(outside the switch block)?
3. It's be nice to find a way to make messages like 'You have unsaved
changes.' translatable. Pass in global js variables via a t('') call?
4. onbeforeunload event should probably be in a if isJsEnabled test,
and should parallel drupal.js's event adding (see addLoadEvent).
5. Comments should be in standard format and in present tense, e.g.,
/**
* Checks to see if a form has been changed after the page loads
*/
6. e_ should be just e to match other js files, e.g., autocomplete.
------------------------------------------------------------------------
Tue, 13 Sep 2005 13:54:35 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_2.js (1.58 KB)
Ok here is an updated JS file.
A few notes, I couldn't get the addSubmitEvent() in drupa.js to
reliably work so I had to set the isSubmit() true in the PHP creation
of the form. If you look at Blogger.com, they do this as well... so
either we both missed an obvious way to do this, or that is the most
practical. Hopefully some wise JS gurus will chime in with an answer.
Same goes for onbeforeunload event, which is completely different then
addEvent defined in drupal.js.
As for the t('') I agree this would be useful but I'm not sure of the
best way to do this. One way would be to put in form() a t('') passed
in, and then write this to a var in formcheck.php which returns a JS
file type. Thoughts?
1
0
13 Sep '05
Issue status update for
http://drupal.org/node/10056
Post a follow up:
http://drupal.org/project/comments/add/10056
Project: Drupal
Version: cvs
Component: node system
Category: feature requests
Priority: normal
Assigned to: Anonymous
Reported by: Anonymous
Updated by: fago
Status: patch (ready to be committed)
+1 for this functionality
i've tested the patch and it looks nice, however node_validate_title()
produces an validation error, where the field is called title, which
isn't suitable if the modules changes the name, as the forum.module
does.
fago
Previous comments:
------------------------------------------------------------------------
Sat, 14 Aug 2004 18:17:11 +0000 : Anonymous
Attachment: http://drupal.org/files/issues/node.set-title-field.patch (2.02 KB)
The default "title" field in a node object is useful but not always
appropriate. For some node types it would be preferable to rename the
field for display to users (e.g., from "title" to "name" for a record
of an organization or business), while in others the title might be
best drawn from a combination of other fields (e.g., first_name and
last_name fields,
for a record of an individual).
This patch enables two new alternatives for node titles:
renamingThe title field can be renamed, e.g., from "title" to "name".
substutingOther fields can be substituted for the title field.
The functionality is implemented through a new proposed node hook,
_title_field($op). There are two possible values for $op: "action"
(either "rename" or "substitute") and "names" (a list of field names to
be used in the title field).
Sample uses:
[?PHP
function example_title_field($node, $op) {
switch ($op) {
case 'action':
$return = 'rename';
break;
case 'names':
$return = array('Name');
}
return $return;
}
?]
In this case, since the requested action is "rename", the title field
would be labelled "Name" (instead of "Title") in node add and edit
forms for nodes of type "example".
[?PHP
function example_title_field($node, $op) {
switch ($op) {
case 'action':
$return = 'substitute';
break;
case 'names':
$return = array('first_name', 'last_name');
}
return $return;
}
?]
In this case, no "Title" field would be displayed in node add or node
edit forms of node type "example". Instead, the fields "first_name"
and "last_name" would be concatenated to form a title.
Implementing this patch would significantly increase the flexibility of
the node system, making it feasible to store information of many types
not currently supported because they don't have a "title" attribute.
------------------------------------------------------------------------
Sun, 15 Aug 2004 16:39:37 +0000 : Dries
This is best accomplished through the existing nodeapi hook, using a
separate module.
------------------------------------------------------------------------
Sun, 15 Aug 2004 23:02:13 +0000 : nedjo
Thanks for the suggestion. I did look for a way to accomplish this
through _nodeapi (and other existing hooks), but couldn't see how. The
"title" field is hard-coded into node.module. The _nodeapi hook allows
various types of manipulations, but not, so far as I could see, changes
to elements that have been generated by, e.g., form_textfield() calls.
Are there possibilities I'm missing? Other approaches? Any specific
suggestions as to means we could use to change the hard-coded "title"
field?
------------------------------------------------------------------------
Thu, 19 Aug 2004 18:27:15 +0000 : nedjo
Attachment: http://drupal.org/files/issues/node.set-title-field_0.patch (741 bytes)
Drawing on Dries's suggestion, I've substantially revised this patch to
enable node title field customization with only three additional lines
of code in node.module.
In place of the existing line in function node_form()
<?php
$output .= form_textfield(t('Title'), 'title', $edit->title, 60, 128, NULL, NULL, TRUE);
?>
the patch substitutes a the following:
<?php
$title = variable_get('node_titlefield_'. $edit->type, 'Title');
if(!($title == 'none')) {
$output .= form_textfield(t($title), 'title', $edit->title, 60, 128, NULL, NULL, TRUE);
}
?>
With this change in place, node modules will be able to override the
standard node title label by setting a variable,
node_titlefield_nodetype, where nodetype is the type of node. This
could be done, e.g., in a _settings hook, so that the first time
settings were accessed the module would set the variable:
<?php
if (variable_get('node_titlefield_example', '') == '') {
// since none is already set, set this variable
variable_set('node_titlefield_example', 'Name');
drupal_set_message('Example module initialization completed.');
}
?>
The special value 'none' would suppress the title field altogether in
node forms, used as follows:
<?php
if (variable_get('node_titlefield_example', '') == '') {
// since none is already set, set this variable
variable_set('node_titlefield_example', 'none');
drupal_set_message('Example module initialization completed.');
}
?>
In this case, a _validate hook would be used to set the title field
value. Assuming a module where a first name-last name combination was
to be used for a title:
<?php
function example_validate(&$node) {
if(!(isset($node->title))) {
$node->title = $node->first_name . ' ' . node->last_name;
}
}
?>
In short, this three-line addition to the node.module would
significantly increase the flexibility of the node system by removing
the hard-coded 'Title' field in node add and edit forms and instead
enabling custom labelling or concatenation of the field from other
sources.
------------------------------------------------------------------------
Thu, 19 Aug 2004 18:50:42 +0000 : moshe weitzman
I like this functionality. But the implementation still seems unclean. I
would prefer to simply remove the 'title' form field from node_form()
and require that modules present this field in whatever way they
please. in some cases, they will use a hidden form field because no
customer interaction is required. This causes a break with backward
compatibility, but cleanliness is facored over compatibilityness (when
you must choose).
I'm moving this back to 'Active', since I don't think this patch is
committable as is.
------------------------------------------------------------------------
Fri, 27 Aug 2004 23:46:45 +0000 : nedjo
Attachment: http://drupal.org/files/issues/node.set-title-field_1.patch (830 bytes)
Thanks for looking at this. The suggestion - dropping the title field
altogether and leaving this for module authors - has the advantage of
simplicity. However, this change would imply significant new work as it
would would require all or nearly all existing modules - core and
contributed - be modified. While I think enabling title field
customization is important for Drupal as a whole, and while this is a
change I need for two modules I'm working on, I'm not immediately
convinced that the benefits of this change justify this cost. Also, in
most cases, the present title field works fine, so keeping it as a
default option would I feel be desirable.
Hence the following patch. In place of my (poorly conceived) initial
hook proposals, above, this patch introduces a single very
straightforward new node hook, _title_field. By setting this hook to
return FALSE, node modules authors will suppress the default title
field, and be free therefore to substitute what they wish or need.
Returning TRUE, or not using the hook, will result in the default title
field being used.
Sample usage:
<?php
function example_title_field() {
return FALSE;
}
?>
This is, I feel, a much better solution than the previous two I
suggested--proof, if it's true, that the Drupal issue system works!
------------------------------------------------------------------------
Mon, 06 Sep 2004 15:50:38 +0000 : nedjo
Are there any objections to or issues raised by this small patch
(introducing a new hook to make the title field optional in node
forms)? If not, can it be applied?
------------------------------------------------------------------------
Tue, 07 Sep 2004 13:46:29 +0000 : Anonymous
I think I agree with Moshe's opinion that this should be accomplished by
having node modules display the title field. In any event, this is a new
feature and shouldn't go in during the feature freeze for 4.5.
------------------------------------------------------------------------
Tue, 07 Sep 2004 13:58:34 +0000 : ccourtne
+1 for just moving the title field to be a responsibility of the
implementing module instead of node_form. There is already to some
extent "hook overload", in addition there may be several modules which
don't want to even display a title module. Why add that only fixes
some of the problems just to have to remove it later when you make a
fix that address the whole problem.
------------------------------------------------------------------------
Tue, 07 Sep 2004 21:09:53 +0000 : nedjo
Attachment: http://drupal.org/files/issues/node.remove-title-field.patch (636 bytes)
Thanks for comments. As per feedback, the attached patch removes the
title field from node forms, leaving this instead to node modules.
Understood that possible approval of this patch this will need to wait
until code unfrozen.
------------------------------------------------------------------------
Sun, 24 Oct 2004 12:06:46 +0000 : moshe weitzman
please patch all core node modules as well.
------------------------------------------------------------------------
Sun, 28 Nov 2004 01:00:31 +0000 : nedjo
Attachment: http://drupal.org/files/issues/node-move-form-title-field.patch (4.29 KB)
Good point Moshe. This patch of node.module plus all core node modules
simply moves the title field out of node.module and into the node
modules, enabling (if desired) customization of the title field by a
module. Patches to contributed modules will in most cases be as simple
as adding the line
<?php
$output = form_textfield(t('Subject'), 'title', $node->title, 60, 128, NULL, NULL, TRUE);
?>
at the beginning of a typename_form() function (and, if necessary
changing an existing $output = to $output .=). In the attached patch,
I've customized the title field for only one of the node types, calling
the title of a forum topic the "Subject". We could consider calling the
title of a blog a "Subject" or "Topic" as well.
I feel this is a good solution (thanks for the feedback and discussion)
and, for minimal work (minor updates to contributed modules), will have
the significant benefit of enabling flexibility in content types beyond
those that have "title" attributes.
------------------------------------------------------------------------
Sun, 28 Nov 2004 17:45:35 +0000 : nedjo
Ability to customize title as provided in this patch was requested here
[1]:
"When adding/editing, the 'title' field is manditory, which is fine.
Except that its label is hardcoded to 'title'. A custom node modelling,
say, a 'Company' would much rather this field be presented to the user
as 'Company Name'.
"
[1] http://drupal.org/node/13596
------------------------------------------------------------------------
Fri, 04 Mar 2005 03:44:55 +0000 : nedjo
Here's another patch that's sat for months without being either applied
or turned down, although there was expressed support and, after several
iterations, it addressed all issues that had been raised. The point is
to remove the hard-coded "Title" field - inapprioriate for many node
types, e.g., companies - from node forms, instead allowing node modules
to treat this as appropriate (e.g., a "Subject", "Name" or "Topic", or
nothing at all).
Are there problems with this approach that were not raised? Is there a
reason this wasn't accepted?
------------------------------------------------------------------------
Fri, 04 Mar 2005 03:54:02 +0000 : Steven
nedjo: if you read the backlog you can see that this feature was delayed
for after the 4.6 release.
"I've customized the title field for only one of the node types,
calling the title of a forum topic the "Subject".
"
This is really inconsistent, as what you call the title of a blog,
story or forum node is purely subjective. "Subject" "Topic" and "Title"
are pretty much interchageable and any subtle differences will surely be
eroded by translation. The only thing I can agree on so far is naming a
polls' title "Question".
------------------------------------------------------------------------
Fri, 04 Mar 2005 05:15:03 +0000 : nedjo
Thanks for the note. I'm not sure what you mean by the "backlog".
Where would I find this? I agree that a "Question" field would make
sense for polls.
------------------------------------------------------------------------
Fri, 04 Mar 2005 14:12:56 +0000 : JonBob
Actually UnConeD, it was postponed until after the 4.5 release, not 4.6,
and looks like everyone forgot about it when the release happened. And
no, Nedjo, I don't take this as proof that Drupal needs more
bureaucracy. :-)
Marking this active again; I'm sure the patch will have to be updated
once the code branches. We had rolled this function into the needs for
CCK, but in our discussions split it off again.
------------------------------------------------------------------------
Sun, 03 Apr 2005 05:28:08 +0000 : chx
Attachment: http://drupal.org/files/issues/node-move-form-title-field_0.patch (4.59 KB)
The patch applies to ucrrent HEAD with some offset. I have rerolled it
so it applies cleanly.
------------------------------------------------------------------------
Fri, 20 May 2005 10:09:50 +0000 : chx
Patch still applies (yes, with some offset, but applies). ANd yes, we
need this very badly.
------------------------------------------------------------------------
Fri, 20 May 2005 10:18:49 +0000 : Thox
I'd benefit greatly from having a feature like this. The example already
given is one of the cases I need it: I'd like the title field to be a
concatenation of two fields: "First name" and "Last name".
------------------------------------------------------------------------
Tue, 24 May 2005 03:39:40 +0000 : gsperk
I wonder if someone could help me with the following problem.
After applying the patch I made the output for 'title' in my
own_node.module this:
$output .= form_hidden('title', $node->firstname . ' ' .
$node->surname);
When the form is first submitted I get the error 'You have to specify a
title'. When I submit again the firstname/surname variables are
obviously in $node, and the form submits with the desired title.
So, I suppose my question is, how can get the values for firstname and
surname into $node without submitting twice? (If it's not obvious I can
confirm that I dont have much experience with this kind of stuff. Any
suggestions would be a big help.)
------------------------------------------------------------------------
Wed, 25 May 2005 04:23:41 +0000 : Steven
To make this patch work you also need to move the title validation out
of node.module and into the node modules. I don't think this is that
much of a problem, it is code that is not going to be touched much.
Also, in light of the upcoming CCK it makes sense to do it like this.
------------------------------------------------------------------------
Sat, 02 Jul 2005 17:02:26 +0000 : hanoii
Nice thread.
I have come up with a slightly different approach I think that would
need less patching/updating of other modules.
Drupal 4.6.2
node.module:1275
<?php
// Get the node-specific bits.
// We can't use node_invoke() because $param must be passed by reference.
$function = node_get_module_name($edit) .'_form';
$param = array();
if (function_exists($function)) {
$form .= $function($edit, $param, $title);
}
?>
Notice the $title param on the hook_form invoke. This was what I added.
As that hook is called like this an not with the node_invoke() I can
define on my custom node module, or patch any module I need to modify
the title description with the following hook:
<?php
function budget_form(&$node, &$params, &$title) {
?>
and modify the $title argument there, as it is by reference, It will me
modified on node.module.
Then again on
node.module:1319
I added/changed the following:
<?php
if ( ! $title ) {
$title = t('Title') ;
}
$output .= form_textfield($title, 'title', $edit->title, 60, 128, NULL, NULL, TRUE);
?>
So, if the module modify the title var, it uses that, if not, it uses
the default one.
Worked for me so far, nice to post on drupal.
Greetings,
a.=
------------------------------------------------------------------------
Sat, 02 Jul 2005 18:10:28 +0000 : hanoii
I posted too soon the thing before... :)
I did further patches because of the validation keep telling that I
have tu put a "title".
I was happy with the previous patch, not so much with this one, but I
let you know what I did...
on the node.module, node_validate() function I move the title
validation block just after the
<?php
// Do node-type-specific validation checks.
node_invoke($node, 'validate');
node_invoke_nodeapi($node, 'validate');
?>
and added another parameter to the form_set_error()
<?php
// Validate the title field.
if (isset($node->title)) {
if (trim($node->title) == '') {
form_set_error('title', t('You have to specify a title.'), TRUE);
}
}
?>
then, on the form_set_error function in common.inc I did the following:
<?php
/**
* File an error against the form element with the specified name.
*/
function form_set_error($name, $message, $checkexistance = FALSE ) {
if ( !$checkexistance || $checkexistance && !isset($GLOBALS['form'][$name]) ) {
$GLOBALS['form'][$name] = $message;
drupal_set_message($message, 'error');
}
}
?>
So now, if any previous title error was added, and the $checkexistance
flag is TRUE, no error will be added to the message queue.
The moving place of the validation block was to let the modules
validate first than the node module.
Errrr.. I don't like it, but again, it worked :)
a.=
------------------------------------------------------------------------
Mon, 01 Aug 2005 00:54:22 +0000 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/title_0.patch (4.56 KB)
I've updated the patch to apply to current cvs. As progress on the CCK
has been rather slow, I'd appreciate if somebody could address Steven's
concerns and the patch could be applied.
------------------------------------------------------------------------
Tue, 16 Aug 2005 19:19:23 +0000 : Thox
Attached is a patch with an attempt to move the title validation code
into the node modules (from node.module).
------------------------------------------------------------------------
Tue, 16 Aug 2005 19:20:21 +0000 : Thox
Attachment: http://drupal.org/files/issues/title-validation.patch (6.22 KB)
attached again (issue preview seems busted?)
------------------------------------------------------------------------
Wed, 24 Aug 2005 18:57:26 +0000 : nedjo
Thanks chx, killes, and Thox for updates and fixes on the patch.
I've applied and tested it and Thox's changes seem to address the title
validation issue.
It's a couple of releases later--it would be great to see this fix
applied!
------------------------------------------------------------------------
Wed, 24 Aug 2005 23:17:25 +0000 : mathias
Attachment: http://drupal.org/files/issues/title-validation_0.patch (6.04 KB)
+1
I've needed to change the name of the title field or even altogether
hide it for custom modules.
Issue preview was working for me.
We should document for module developers the implications title-less
nodes (e.g., less informative watchdog entries, harder to edit nodes,
node title lists become buggy, etc).
I couldn't get the patch to apply cleanly to HEAD so here's a new one.
------------------------------------------------------------------------
Mon, 12 Sep 2005 15:42:30 +0000 : Robrecht Jacques
Attachment: http://drupal.org/files/issues/title-validation_1.patch (6.23 KB)
+1 I need this too: I want to be able to set the title of a node
automatically.
Patch still applies with offset (rerolled patch attached).
1
0
[drupal-devel] [feature] Prevent accidentally navigating away from pages where content has changed
by m3avrck 13 Sep '05
by m3avrck 13 Sep '05
13 Sep '05
Issue status update for
http://drupal.org/node/30220
Post a follow up:
http://drupal.org/project/comments/add/30220
Project: Drupal
Version: cvs
Component: base system
Category: feature requests
Priority: normal
Assigned to: Anonymous
Reported by: m3avrck
Updated by: m3avrck
Status: patch (code needs review)
Attachment: http://drupal.org/files/issues/formcheck_2.js (1.58 KB)
Ok here is an updated JS file.
A few notes, I couldn't get the addSubmitEvent() in drupa.js to
reliably work so I had to set the isSubmit() true in the PHP creation
of the form. If you look at Blogger.com, they do this as well... so
either we both missed an obvious way to do this, or that is the most
practical. Hopefully some wise JS gurus will chime in with an answer.
Same goes for onbeforeunload event, which is completely different then
addEvent defined in drupal.js.
As for the t('') I agree this would be useful but I'm not sure of the
best way to do this. One way would be to put in form() a t('') passed
in, and then write this to a var in formcheck.php which returns a JS
file type. Thoughts?
m3avrck
Previous comments:
------------------------------------------------------------------------
Thu, 01 Sep 2005 22:47:42 +0000 : m3avrck
How many times have you been posting a comment or working on a new post
when you click on a link in another application and you navigate away
from the content you are editing, losing all of your changes?
Well I say we introduce a script, much similiar to that of Blogger,
that uses the Javascript window.onbeforeunload handler to prevent this
from happening. We can set this handler to call a function that
compares the contents of all forms when the page loads and then quickly
compares that to the current contents just before the user is about to
leave the page. If they have changed, we should prompt the user so they
don't lose their work. This would save many headaches and degrade nicely
for users without Javascript.
Additionally, this script should work with the newly introduced
JS-based upload feature to prevent navigating away as a file is being
uploaded as well.
If I have sometime I'll start work on a patch tomorrow, just wanted to
get the idea into the queue right now :)
------------------------------------------------------------------------
Sat, 10 Sep 2005 23:48:33 +0000 : StuartDH
Sounds like a great feature to me. We regularly get authors and editors
telling us that they've lost the last hour of work by accidentally
navigating away, and I've even just had one of them email me about it a
few minutes ago, so it'd be great if you could put something together
for this.
Cheers
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:33:58 +0000 : MichelleC
Sounds great to me, too. I often have several tabs open doing stuff and
it's easy to lose my place and leave a page with unfinished edits.
Would this alert you when you're going to close an unsaved tab/window
as well?
Michelle
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:34:19 +0000 : MrMattles
Great for us multitaskers that rush and click the wrong thing
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:36:52 +0000 : chx
Very strong -1 as this is IE only.
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:38:40 +0000 : praseodymium
Konqueror already does this, +1 for the idea in general.
------------------------------------------------------------------------
Mon, 12 Sep 2005 15:07:21 +0000 : webchick
Yeah, I think chx is correc that onbeforeunload is an extension
developed by MS [1] (it's not in the ECMAScript specification [2].
However, I found out that this is implemented in Mozilla since 1.7 [3]
(which corresponds to Firefox 1.0, I think?) and I've tested
Blogger.com's version of this functionality in:
Firefox 1.0.6 (working)
Firefox Deer Park Alpha 1 (working)
Opera 8.0.2 (working)
Safari 1.3 (does NOT work)
Safari 2.0.1 (working)
So it seems to be a 'de facto' standard if nothing else.
[1]
http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onbeforeun…
[2]
http://www.ecma-international.org/publications/standards/Ecma-262.htm
[3] http://www.mozilla.org/status/2004-03-01.html
------------------------------------------------------------------------
Mon, 12 Sep 2005 15:09:15 +0000 : webchick
Also, I should clarify... that "does NOT work" in Safari doesn't display
any errors or anything, it just doesn't save the form results as
expected (so pre-Tiger Safari users are going to be used to this). So I
don't see any harm in including this functionality since it seems to
degrade gracefully in non-supporting browsers.
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:10:18 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/node.module_11.patch (951 bytes)
And a patch is ready! Code was based on many example, including this [4]
and this [5] along with adding my own thoughts and what not ;)
[4]
http://www.codestore.org/store.nsf/cmnts/451FA051398A9AF486256EE0000FB7D1?O…
[5] http://www.blogger.com/app/scripts/formcheck.js
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:10:45 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck.js (1.54 KB)
And here is the JS to throw in misc/.
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:22:56 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_0.js (1.61 KB)
Better JS file attached without tabs. Also, tested and working great in
FF and IE6. Doesn't work in Opera 8, however no errors are present, I
believe this is just because the event handler is not supported. If
there is a work around for Opera, please let me know!
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:24:18 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_1.js (1.61 KB)
One more try with those tabs.
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:32:48 +0000 : moshe weitzman
hmmm. i usually find these prompts annoying. any chance we can attach
the behavior only to the node and comment forms? those are the "high
risk" areas. what do others think?
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:42:39 +0000 : m3avrck
Yes, the JS is only loaded on the node edit/create pages, won't affect
any admin/login/etc form. Also, the prompt is unobstrusive and if you
goto save a node you don't get a warning that the contents have changed
because you are explicity saving the node (hence no reason!).
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:53:01 +0000 : Dries
I want some JS-folks to review the code.
------------------------------------------------------------------------
Mon, 12 Sep 2005 22:09:10 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/drupal_15.patch (1.94 KB)
After talking with Drumm on IRC, this patch makes the integration with
Drupal much simpler and it's off by default. Only turned on for the
node edit/change page right now but any module/form can easily add this
by passing a 'TRUE' parameter.
------------------------------------------------------------------------
Mon, 12 Sep 2005 23:38:35 +0000 : nedjo
+1 on idea, js looks generally good, a few suggestions:
1. Instead of writing onsubmit via PHP, use a js addLoadEvent call.
2. Shouldn't the return false in isElementChanged be at the end
(outside the switch block)?
3. It's be nice to find a way to make messages like 'You have unsaved
changes.' translatable. Pass in global js variables via a t('') call?
4. onbeforeunload event should probably be in a if isJsEnabled test,
and should parallel drupal.js's event adding (see addLoadEvent).
5. Comments should be in standard format and in present tense, e.g.,
/**
* Checks to see if a form has been changed after the page loads
*/
6. e_ should be just e to match other js files, e.g., autocomplete.
1
0
Issue status update for
http://drupal.org/node/27364
Post a follow up:
http://drupal.org/project/comments/add/27364
Project: Drupal
Version: cvs
Component: filter.module
Category: tasks
Priority: normal
Assigned to: Bèr Kessels
Reported by: Bèr Kessels
Updated by: Bèr Kessels
-Status: patch (code needs work)
+Status: patch (ready to be committed)
Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_2_0.patch (13.59 KB)
changed that line of help.
Can this please still be taken in consideration before the freeze?
Bèr Kessels
Previous comments:
------------------------------------------------------------------------
Sun, 24 Jul 2005 09:30:54 +0000 : Bèr Kessels
This is a mockup for the Filter UI. Currently it is soo complex, that I
dare to call it broken.
Please comment on this, for I will not ake any patches, when the
general idea is disliked.
The idea is t split filters and input formats better. Filters are
filters, defined in modules. Input formats are bundles of filters. This
is how we have it ATM, but the interface fails to communicate that. I
tried to develop a consistent (with the rest of Drupal) interfce that
makes it clearer what is what.
The menu is as follows:
* administer
** ....
** settings
*** input formats
*** filters
** ...
------------------------------------------------------------------------
Sun, 24 Jul 2005 09:37:10 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-formats_list1.png (16.34 KB)
* administer
** ....
** settings
*** input formats >> see attachement 1
*** filters
**
------------------------------------------------------------------------
Sun, 24 Jul 2005 09:39:02 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-formats_add.png (23.42 KB)
* administer
** ....
** settings
*** input formats >> see attachement 2, tab 2
*** filters
**
Note: The default checkbox lmay seem a bit odd. But checking it will
remove the "default status" from the current "default" one and make
this one "default". The help text, and a drupal_set_message should
explain this.
------------------------------------------------------------------------
Sun, 24 Jul 2005 09:40:08 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-formats_edit_format.png (22.17 KB)
* administer
** ....
** settings
*** input formats >> clicking a 'configure' link in the table of
attachement 1
*** filters
**
------------------------------------------------------------------------
Sun, 24 Jul 2005 09:40:55 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-filters_list.png (36.24 KB)
* administer
** ....
** settings
*** input formats
*** filters >> see attachement 4
**
------------------------------------------------------------------------
Sun, 24 Jul 2005 09:41:40 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-formats_edit.png (22.63 KB)
* administer
** ....
** settings
*** input formats
*** filters >> clicking a 'configure' link in the table of attachement
4
**
------------------------------------------------------------------------
Mon, 25 Jul 2005 14:46:27 +0000 : Bèr Kessels
IMO this makes the interface easier. But other disagree. And because it
also removes one feature: (being able to use E.g. HTML in different
input formats, with different settings)
I'll simply wont fix this :(
------------------------------------------------------------------------
Thu, 28 Jul 2005 13:36:12 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy.patch (10.28 KB)
I decided to go for it anyway. Be it in a slightly different way then my
initial mockups. The difference is that I left the filters where they
are, hidden beneath the formats.
So, here is the patch that makes the filter UI more consistent with
screens like blocs, menus, vocabularies et al
also nice to note is that:
93 lines are added, 104 are removed. So netto we have less code :)
(cvs diff filter.module | grep ^+ | wc -l)
------------------------------------------------------------------------
Thu, 28 Jul 2005 13:37:02 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-formats_list_html.png (44.64 KB)
Here is how the listing now looks.
------------------------------------------------------------------------
Thu, 28 Jul 2005 13:37:57 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-formats_edit_format_html.png (113.71 KB)
And the "configure" screen.
The 'add' screen looks exactly the same, only with the default values
pre-filled.
------------------------------------------------------------------------
Thu, 28 Jul 2005 13:39:19 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-formats_edit_format_too_long.png (121.82 KB)
Oh, And here is a really nice example of why the current screen is no
good.
And here I "only" have four roles. I run a site with 12 roles, imagine
this screen then!
------------------------------------------------------------------------
Thu, 28 Jul 2005 13:48:35 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_HEAD.patch (10.24 KB)
And here is a pathc for HEAD (I forgot that I developed this on a 4.6.2
codebase. Sorry)
------------------------------------------------------------------------
Thu, 28 Jul 2005 13:55:25 +0000 : stefan nagtegaal
I like this very, very much!
This is - indeed - much better than the current UI. This always fits,
and makes the life of people who use a fixed width theme much nicer..
Ber, FYI (you probably know this too), 'admin/access' does also break
fixed width themes due to the fact that it expands the width of the
table when more roles are being added..
Good catch! I like it much more this way..
+1 from me...
------------------------------------------------------------------------
Fri, 29 Jul 2005 07:27:34 +0000 : Dries
The code style needs work (spaces, tabs, 'as' -> 'AS'), the code
comments need work, and debug statements should be removed.
Screenshots look good to me. db_query('UPDATE {filter_formats} SET
cache = %d, name = \'%s\', roles = \'%s\' WHERE format = %d',
(int)$cache, $name, $roles, $format); can be written better/shorter
quote-wise.
------------------------------------------------------------------------
Fri, 29 Jul 2005 09:04:16 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_HEAD_0.patch (10.37 KB)
Dries, thanks a lot for the review. OI fixed your comments. Or so I
hope. code-style.pl was of no use (can that *please* be removed from
core?) because it chokes on the help texts. It also pointed out a lot
of old formatting isues, whic, when fixed wuold flood this patch with
unrelated fixes. I tried to narow my style-searching to the areas I
touched. And I made the comments more verbous.
The query is fixed. it saves no characters, but looks better now.
Thanks for the tip.
The print_r..... blush. thats what I get for not running my default
grep -r print_r on my code after finishing up...
I could not find a lot of spaces and tabs. So it lmight be that i
missed one. I triple checked it though.
------------------------------------------------------------------------
Fri, 29 Jul 2005 09:32:21 +0000 : Steven
Moving the role settings inward does fix the wide-table problem, but it
makes the filter configuration much more opaque. We could perhaps fix
this by showing the roles as a comma-separate list in the input formats
table. Such a list can wrap and will not stretch the table.
Other than that:
* Having the instructions for the Roles and Filters form_group() at the
bottom is a bit weird. The distance is too big.
* The table that was used for toggling filters on/off was much more
compact before, and IMO a lot clearer. Why change to a flat list with
the description staggered below each item?
------------------------------------------------------------------------
Fri, 29 Jul 2005 10:31:30 +0000 : Bèr Kessels
consistancy, consistancy and more consistancy.
There really is nothing worse that having a different 'concept' of UI
for every thing in Drupal. We are going in the right direction, with
the blocks being editable objects, menus acting like that, taxonomy,
users, etc..
I wanted to make this act similar. I am not at all happy with the fact
tah the list is still a form. But we need this, for setting the
default.
A big -1 for a comma-separate list. That is extremely hackish, even
worse useabilitywise and just silly
* we should avoid typing when not needed.
* we should avoid errors on input, when they can be avoided (a typo
is made very easy, I know all about it) Form set errors won't fix that
usability -wise.
I agree that you sort-of loose the overview. But IMO that is a minor
loss comared to what we gain, usability-wise.
The wtoo-wide was onlywhat triggered me. But do not think that I am
noly making this patch to fix that issue. This patch is there to
improve usability, through consistancy. o make drupal behave the same
in most places in admin. If you know one screen, you know most of them.
We really should move away from custom-hacks for every page, because
that is useability rule #1: consistancy. Stadardisation, even if
sometimes a custom hack might make things easier in that single case,
it will reduce your overall usability so much that nearly ever it is
worth that hack.
That select-boxes instead of the table issue: consistancy. As well that
it completely broke fluid CSS layouts, my solution is more consistant,
and uses forms the way they are meant. If we cannot use $descritption
in checkboxes they should be removed. but IMO it feels much more
consistant. Tables are for tabular data, and this is simply a list of
items, with additional data.
And i do not really get what you mean with "too far down". Do you mean
that you would like to see the order of the groups different?
------------------------------------------------------------------------
Fri, 29 Jul 2005 13:31:24 +0000 : Dries
I haven't tested Ber's patch yet but looking at the screenshots, it like
what I see. I remember being confused by the current filter UI, and
often, I still am.
------------------------------------------------------------------------
Fri, 29 Jul 2005 19:53:26 +0000 : moshe weitzman
I dislike the recent movement away from overview pages. I think we can
still provide and overview while allowing editing to happen in a
dedicated form. An example in the wrong direction is 'content types'
admin. It is impossible to get an overview, and the listing page is
worthless. I agree with Steven on this one.
It is true that consistency is desirable. But consistent crap is not.
------------------------------------------------------------------------
Fri, 29 Jul 2005 20:02:39 +0000 : killes(a)www.drop.org
I agree with Moshe. Getting a quick overview about what is set how is
essential for an admin.
------------------------------------------------------------------------
Sat, 30 Jul 2005 00:27:15 +0000 : Robrecht Jacques
Overall I like the screenshots. Did not test the patch.
Bèr, what I think Steven (#15) meant by "showing the roles as a
comma-separate list in the input formats table" is that as an overview
page it would be better if you added a list (not an input box!) to
screenshot "input-formats_list1.png" (comment #1). Add a column that
reads "Roles" (or "Enabled for") and then for each row a comma
seperated list of the roles that can use this input format (or maybe
even better a HTML list). You would still need to click "configure" to
_change_ the roles.
The "Default" text (not a radio button) you already show in the
"Options" column is something similar: it immediately shows that this
input format is the default without you having to go to the "configure"
page. Why not do the same for "roles"?
If this is what Steven meant, then I tend to agree with him.
And to help moshe and killes: maybe you could even add a column that
_lists_ the used input filters. Again, only a list, not a way to change
it. That's what the "operations" column is for.
A table like that will "fluid" better than what we have now, still it
is a _complete_ overview of the settings.
I could have misunderstand someone... I have the tendency to do that...
------------------------------------------------------------------------
Mon, 22 Aug 2005 08:21:32 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_interface_improvements.patch (14.52 KB)
by popular demand: a comma separated list.
------------------------------------------------------------------------
Mon, 22 Aug 2005 08:24:31 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/format_now_contains_roles.png (22.09 KB)
and a screenshot
------------------------------------------------------------------------
Mon, 22 Aug 2005 18:39:44 +0000 : Dries
Please check your use of print theme('page', $output). Normally, you
just return $output.
------------------------------------------------------------------------
Mon, 29 Aug 2005 18:54:28 +0000 : Bèr Kessels
All instances of theme('page',...) in filter.module are now changed to
return ... ;
furthermore, the patch is the same.
------------------------------------------------------------------------
Mon, 29 Aug 2005 20:04:02 +0000 : Uwe Hermann
Bèr, I think you forgot to attach your updated patch.
------------------------------------------------------------------------
Mon, 29 Aug 2005 20:27:39 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_interface_improvements_2.patch (13.52 KB)
my email client has this nice warning system for attachements. Maybe
drupal should search for the word attachement too, and when no att.
found give an error ;). Or maybe i should just learn to pay attention.
Guess that is easier.
Anyway, here it is.
------------------------------------------------------------------------
Thu, 08 Sep 2005 20:22:54 +0000 : Dries
I wanted to apply this patch but:
1. I can't change the default input format. 'Save'-button doesn't
work.
2. I got confused by the fact I can't change the roles of the default
filter. I think the form group description should explain this.
3. form_group(t(filter)) should be form_group(t('filter'), ...).
------------------------------------------------------------------------
Thu, 08 Sep 2005 20:51:24 +0000 : m3avrck
Found a few more issues:
If I go into 'Configure' for 'PHP Code' and click the 'Configure' tab
at the top and then click 'list filters' link in the text, I get this
warning with PHP5 warning: Missing argument 1 for filter_admin_format()
in \drupal\modules\filter.module on line 378.
Also, it is sort of confusing as to what the 'List' tab implies. For
example, if I click on 'input formats' in the admin menu, I get a list
of the current filters. When I click 'configure' for one of those, I
see the options for that filter. However, it says 'list' in one of the
tab, which doesn't really mean much. I thought that was the list of
filters, not the options for that filter. That should be cleaned up.
Also, there is no way to get back to the full list of filters unless
you click on 'input formats' in the admin menu. A setting/link to fix
this would be great.
Also, where is the link to add filters???
I think the options/layout/tabs should mimic of that of admin > users.
So on admin > input formats, it has the tabs 'list', 'add format' ...
very clear and consistent.
On a configure page for a filter, it should have 'list', 'view',
'configure', 'rearrange' ... this would make it easier to navigate and
get back to the original list of filters.
Make sense? I think this and Dries' comments put this patch over the
top :)
------------------------------------------------------------------------
Fri, 09 Sep 2005 01:48:55 +0000 : tag
In my initial battle to figure out this module's (quite nice)
functionality, what tripped me up the most was actually the
nomenclature.
The way I keep it straight in my head now is to translate "input
format" to "filter set". If you view those (the current) screens with
this in mind, I think it's a lot easier to understand what's what. It
sort of describes this in the help text, but the terms "filter" and
"input format" don't really convey the relationship between the two --
there's no way to intuit that an "input format" is a collection of
"filters". Not to my mind at least... and well, we know nobody reads
instructions...
Are there any technicalities that make renaming "input format" to
"filter set" not accurate? Or does someone have a better term/terms to
better show the relationship of these elements? (I'm aware of filters
in servlet architectures but I don't think there's much of a collision
with that usage).
Anyhow, I guess this doesn't quite address the UI controls and/or
layout, but would still help a lot.
------------------------------------------------------------------------
Fri, 09 Sep 2005 06:35:34 +0000 : Bèr Kessels
I prefer filter set. In fact: I like it a lot. Anyone else ideas on
this?
------------------------------------------------------------------------
Fri, 09 Sep 2005 10:18:18 +0000 : Dries
I too had problems with the terminology; it took me months to get used
to 'input formats' versus 'filters'. Using 'filter sets' might improve
the situation -- especially from an administrator's point of view when
you have to wrap your head around the UI/difference. I'm not native
English, but 'filter set' sounds more explanatory, yet slightly more
geeky so I'm not sure if it flies for users who don't need to
understand the underlying concepts (eg. under the node and comment
submission forms).
------------------------------------------------------------------------
Fri, 09 Sep 2005 11:00:05 +0000 : Bèr Kessels
a heads up: I ma redoing the patch. But decided no to change the name
yet. That is a too big change. IT should be in a separate issue, IMO.
------------------------------------------------------------------------
Fri, 09 Sep 2005 15:27:43 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_2.patch (13.57 KB)
* Bugs as reported by dries fixed.
* The [add] tab not appearing is due to your menu caching, refresh that
please.
* Fixed an additional bug: Some agents do not send TRUE for disabled
checkboxen. Also in current filter admin.
and people: we have a problem. Deleting a filter trows errors, due to
the revisions changes. that happens in filter module now, but also
after this patch. I have too little knowledge of the filter system to
fix that, and IMO that is a separate issue. It should be fixed right
after this patch is committed.
------------------------------------------------------------------------
Fri, 09 Sep 2005 15:57:18 +0000 : Bèr Kessels
FYI: the delete bug is waiting here: http://drupal.org/node/30781
------------------------------------------------------------------------
Fri, 09 Sep 2005 18:18:17 +0000 : m3avrck
Get this warning in PHP5: Warning: Missing argument 1 for
filter_admin_format() in \modules\filter.module on line 379
I have traced this error back to line 240. Why are there no callback
arguments like above on line 235? Looks like a source of a problem/bug
here so needs to be addressed, not sure exactly how to fix it myself
(otherwise I would). Should be easy it seems.
Also, for a quick and easy usability improvement, on line 239, change
t('list') to t('view') ... makes the menus less confusing.
After those fixes, looks like this patch is ready to be committed :)
------------------------------------------------------------------------
Fri, 09 Sep 2005 18:23:27 +0000 : m3avrck
Also, this dialog is a bit confusing:
"Choose which roles may use this filter format
You are editing the default format. For the default format, all roles
must be enabled. Therefore you cannot change them.
"
Maybe make it so that first line doesn't appear there on that page?
1
0
[drupal-devel] [feature] Display comments on own tab like wikipedia (patch attached)
by Bèr Kessels 13 Sep '05
by Bèr Kessels 13 Sep '05
13 Sep '05
Issue status update for
http://drupal.org/node/24804
Post a follow up:
http://drupal.org/project/comments/add/24804
Project: Drupal
Version: cvs
Component: comment.module
Category: feature requests
Priority: normal
Assigned to: chx
Reported by: moshe weitzman
Updated by: Bèr Kessels
Status: patch (code needs review)
Discussion about a patch should go further then just comments on the
code. NSK (and I) just did what the patch review guidelines say under
"Challenge the proposed changes.".
Bèr Kessels
Previous comments:
------------------------------------------------------------------------
Sat, 11 Jun 2005 20:57:10 +0000 : moshe weitzman
Attached please find a demonstration patch which moves comment display
to its own tab on the node view page. Admin may choose this new view or
old view.
The patch needs work still:
- comment control form redirects to the node page after submit. Should
be comment page
- same for comment posting form
- probably more
I hope someone gets inspired to finish this patch. I won't revisit this
for a while.
------------------------------------------------------------------------
Sat, 11 Jun 2005 21:00:56 +0000 : moshe weitzman
Attachment: http://drupal.org/files/issues/discuss.patch (3.87 KB)
with attachment this time
------------------------------------------------------------------------
Sat, 11 Jun 2005 23:24:12 +0000 : moshe weitzman
oh yeah - also on the TODO list is to comment URLs to always point to
comment.module and never to node/x#comment-cid or node/x#comment-new
and so on. then comment.module can work out whether to show you the
discuss tab or the traditional view.
------------------------------------------------------------------------
Sun, 12 Jun 2005 00:56:02 +0000 : Boris Mann
I like the concept. Would want this to be part of workflow settings, so
that comment-tab could be enabled/disabled per content type.
------------------------------------------------------------------------
Sun, 12 Jun 2005 02:20:42 +0000 : Jaza
I like the idea - big +1. This is a must for sites that have long
articles and/or many comments - viewing the node is enough bandwidth as
it is, without all the comments getting displayed as well. Also improves
usability, IMO.
Would also like to see pagination of the comments tab (I'm sure you've
thought of this already, Moshe). Something similar to A List Apart
[1]'s system, e.g. node/x/discuss/2 (page 2 of that node's comments).
Or, if the existing pager system demands it, node/x/discuss?page=2.
[1] http://www.alistapart.com/
------------------------------------------------------------------------
Tue, 30 Aug 2005 00:41:42 +0000 : chx
Attachment: http://drupal.org/files/issues/discuss_0.patch (3.87 KB)
Let's discuss! Like do we want to have a reply form on the node display
even if the comments are on the discuss page?
------------------------------------------------------------------------
Tue, 30 Aug 2005 00:50:13 +0000 : chx
Attachment: http://drupal.org/files/issues/discuss_1.patch (3.58 KB)
I have uploaded moshe's patch. Let's try again.
------------------------------------------------------------------------
Tue, 30 Aug 2005 11:22:18 +0000 : Bèr Kessels
I like this approach a lot. However, as i use drupal also for blogging,
would dislike my bog to behave the same. Thus there must be some -easy-
way to get around this. µ
The problem lies mainly in the fact that tabs are not administratable
in Drupals menu admin. And I think it is a very bad idea to add a
config option where to place the comments. We should go for one way,
teh default way, and offer a menu-admin of some sort to change this,
for bloggers and slashdot alikes.
How to achieve this? I see two options, but I really hope people see
more then one.:
- make local-tasks appear in the menu admin (gets a huge +1 from me)
It's a separate issue, though.
- add a config option to the comment admin to allow comments either as
tab or as thread under the post. IMO that config option should not set
a variable, that gets checked every hook_menu call, but it should alter
the menu-entry in the database for the tabs Its possible, I did that
before.
But, as said, I hope others have better ideas. for I really would love
this feature to get in. But I am afraid a huge lot of people will move
away from drupal if we allow tabs only.
------------------------------------------------------------------------
Tue, 30 Aug 2005 14:05:59 +0000 : chx
No, not tabs only. I know the drupal_gotos are giving you this
impression, but will fix in the evening. Also, there is only a
variable_get for node/nid pages and then again, variable_get is double
cached, it's very fast.
------------------------------------------------------------------------
Tue, 30 Aug 2005 16:19:22 +0000 : Bèr Kessels
I know variable is fast, but I was referring to:
&& variable_get('comment_task', FALSE) that is evaluated every node
view.
As well as the setting in the comment options. I am no fan of such
options; However, it seems in this case that setting is sortof
required, unfortunately. So unless others come up with better ideas,
just ignore my previous comment/post.
Another idea: put this in hook_nodeapi() $op == "settings". that way
this setting can be used per type. I can imagine that this makes no
sense for forums and blog entries, yet is very usefull for the books on
that same site.
------------------------------------------------------------------------
Tue, 30 Aug 2005 16:20:36 +0000 : Bèr Kessels
sorry, with "Another idea: put this in..." I meant to say "Another idea:
put the settings for tabs vs threads ..."
------------------------------------------------------------------------
Mon, 12 Sep 2005 22:24:22 +0000 : chx
Attachment: http://drupal.org/files/issues/discuss_2.patch (6.71 KB)
Comment URL is introduced. Instead of node/$nid#comment-cid we have
comment/$nid#comment-cid
Other minor fixes.
------------------------------------------------------------------------
Tue, 13 Sep 2005 00:31:31 +0000 : Boris Mann
Comment URLs are great, except I'd love to have a *full* comment URL --
e.g. comment/$nid/$cid. The probably with anchor tags is that they are
not visible at the server level -- i.e. not tracked in referrers, etc.
etc.
Don't want to hijack this issue, so feel free to ignore for now, but
wanted to bring up the issue from a search/web traffic/etc.
perspective.
------------------------------------------------------------------------
Tue, 13 Sep 2005 05:51:09 +0000 : nedjo
+1 in principle. (I've looked over but not applied the code.)
(Optional) tabbed display is a useful enhancement and justifies the
extra configuration option. I also like the new comment URL.
------------------------------------------------------------------------
Tue, 13 Sep 2005 05:59:36 +0000 : nsk
As a user I have no problem with this as long as I can disable it and
use the old view. Please note that there is nothing special or
innovative in Wikipedia's use of the comment page (talk page), it's
just a feature of MediaWiki, the software they use. Many other sites
use the same software. IMO it's bad but others may need, so it's
probably good to add something similar in Drupal as long as it's not
the default.
------------------------------------------------------------------------
Tue, 13 Sep 2005 06:06:20 +0000 : moshe weitzman
@nsk (and others) - please do not post simple "like/dislike" comments
when an issue is in 'rpatch: code needs review' status. the review
guidelines are at http://drupal.org/patch/review. thank you.
1
0
[drupal-devel] [feature] Display comments on own tab like wikipedia (patch attached)
by moshe weitzman 13 Sep '05
by moshe weitzman 13 Sep '05
13 Sep '05
Issue status update for
http://drupal.org/node/24804
Post a follow up:
http://drupal.org/project/comments/add/24804
Project: Drupal
Version: cvs
Component: comment.module
Category: feature requests
Priority: normal
Assigned to: chx
Reported by: moshe weitzman
Updated by: moshe weitzman
Status: patch (code needs review)
@nsk (and others) - please do not post simple "like/dislike" comments
when an issue is in 'rpatch: code needs review' status. the review
guidelines are at http://drupal.org/patch/review. thank you.
moshe weitzman
Previous comments:
------------------------------------------------------------------------
Sat, 11 Jun 2005 20:57:10 +0000 : moshe weitzman
Attached please find a demonstration patch which moves comment display
to its own tab on the node view page. Admin may choose this new view or
old view.
The patch needs work still:
- comment control form redirects to the node page after submit. Should
be comment page
- same for comment posting form
- probably more
I hope someone gets inspired to finish this patch. I won't revisit this
for a while.
------------------------------------------------------------------------
Sat, 11 Jun 2005 21:00:56 +0000 : moshe weitzman
Attachment: http://drupal.org/files/issues/discuss.patch (3.87 KB)
with attachment this time
------------------------------------------------------------------------
Sat, 11 Jun 2005 23:24:12 +0000 : moshe weitzman
oh yeah - also on the TODO list is to comment URLs to always point to
comment.module and never to node/x#comment-cid or node/x#comment-new
and so on. then comment.module can work out whether to show you the
discuss tab or the traditional view.
------------------------------------------------------------------------
Sun, 12 Jun 2005 00:56:02 +0000 : Boris Mann
I like the concept. Would want this to be part of workflow settings, so
that comment-tab could be enabled/disabled per content type.
------------------------------------------------------------------------
Sun, 12 Jun 2005 02:20:42 +0000 : Jaza
I like the idea - big +1. This is a must for sites that have long
articles and/or many comments - viewing the node is enough bandwidth as
it is, without all the comments getting displayed as well. Also improves
usability, IMO.
Would also like to see pagination of the comments tab (I'm sure you've
thought of this already, Moshe). Something similar to A List Apart
[1]'s system, e.g. node/x/discuss/2 (page 2 of that node's comments).
Or, if the existing pager system demands it, node/x/discuss?page=2.
[1] http://www.alistapart.com/
------------------------------------------------------------------------
Tue, 30 Aug 2005 00:41:42 +0000 : chx
Attachment: http://drupal.org/files/issues/discuss_0.patch (3.87 KB)
Let's discuss! Like do we want to have a reply form on the node display
even if the comments are on the discuss page?
------------------------------------------------------------------------
Tue, 30 Aug 2005 00:50:13 +0000 : chx
Attachment: http://drupal.org/files/issues/discuss_1.patch (3.58 KB)
I have uploaded moshe's patch. Let's try again.
------------------------------------------------------------------------
Tue, 30 Aug 2005 11:22:18 +0000 : Bèr Kessels
I like this approach a lot. However, as i use drupal also for blogging,
would dislike my bog to behave the same. Thus there must be some -easy-
way to get around this. µ
The problem lies mainly in the fact that tabs are not administratable
in Drupals menu admin. And I think it is a very bad idea to add a
config option where to place the comments. We should go for one way,
teh default way, and offer a menu-admin of some sort to change this,
for bloggers and slashdot alikes.
How to achieve this? I see two options, but I really hope people see
more then one.:
- make local-tasks appear in the menu admin (gets a huge +1 from me)
It's a separate issue, though.
- add a config option to the comment admin to allow comments either as
tab or as thread under the post. IMO that config option should not set
a variable, that gets checked every hook_menu call, but it should alter
the menu-entry in the database for the tabs Its possible, I did that
before.
But, as said, I hope others have better ideas. for I really would love
this feature to get in. But I am afraid a huge lot of people will move
away from drupal if we allow tabs only.
------------------------------------------------------------------------
Tue, 30 Aug 2005 14:05:59 +0000 : chx
No, not tabs only. I know the drupal_gotos are giving you this
impression, but will fix in the evening. Also, there is only a
variable_get for node/nid pages and then again, variable_get is double
cached, it's very fast.
------------------------------------------------------------------------
Tue, 30 Aug 2005 16:19:22 +0000 : Bèr Kessels
I know variable is fast, but I was referring to:
&& variable_get('comment_task', FALSE) that is evaluated every node
view.
As well as the setting in the comment options. I am no fan of such
options; However, it seems in this case that setting is sortof
required, unfortunately. So unless others come up with better ideas,
just ignore my previous comment/post.
Another idea: put this in hook_nodeapi() $op == "settings". that way
this setting can be used per type. I can imagine that this makes no
sense for forums and blog entries, yet is very usefull for the books on
that same site.
------------------------------------------------------------------------
Tue, 30 Aug 2005 16:20:36 +0000 : Bèr Kessels
sorry, with "Another idea: put this in..." I meant to say "Another idea:
put the settings for tabs vs threads ..."
------------------------------------------------------------------------
Mon, 12 Sep 2005 22:24:22 +0000 : chx
Attachment: http://drupal.org/files/issues/discuss_2.patch (6.71 KB)
Comment URL is introduced. Instead of node/$nid#comment-cid we have
comment/$nid#comment-cid
Other minor fixes.
------------------------------------------------------------------------
Tue, 13 Sep 2005 00:31:31 +0000 : Boris Mann
Comment URLs are great, except I'd love to have a *full* comment URL --
e.g. comment/$nid/$cid. The probably with anchor tags is that they are
not visible at the server level -- i.e. not tracked in referrers, etc.
etc.
Don't want to hijack this issue, so feel free to ignore for now, but
wanted to bring up the issue from a search/web traffic/etc.
perspective.
------------------------------------------------------------------------
Tue, 13 Sep 2005 05:51:09 +0000 : nedjo
+1 in principle. (I've looked over but not applied the code.)
(Optional) tabbed display is a useful enhancement and justifies the
extra configuration option. I also like the new comment URL.
------------------------------------------------------------------------
Tue, 13 Sep 2005 05:59:36 +0000 : nsk
As a user I have no problem with this as long as I can disable it and
use the old view. Please note that there is nothing special or
innovative in Wikipedia's use of the comment page (talk page), it's
just a feature of MediaWiki, the software they use. Many other sites
use the same software. IMO it's bad but others may need, so it's
probably good to add something similar in Drupal as long as it's not
the default.
1
0
Hi!
auction module uses mysql_fetch_array
download module uses mysql_escape_string
media module uses mysql_escape_string
recipe module uses mysql_insert_id
Given the age and useability of auction and download I suggest their
removal.
Regards
NK
1
0
[drupal-devel] [feature] Display comments on own tab like wikipedia (patch attached)
by nsk 13 Sep '05
by nsk 13 Sep '05
13 Sep '05
Issue status update for
http://drupal.org/node/24804
Post a follow up:
http://drupal.org/project/comments/add/24804
Project: Drupal
Version: cvs
Component: comment.module
Category: feature requests
Priority: normal
Assigned to: chx
Reported by: moshe weitzman
Updated by: nsk
Status: patch (code needs review)
As a user I have no problem with this as long as I can disable it and
use the old view. Please note that there is nothing special or
innovative in Wikipedia's use of the comment page (talk page), it's
just a feature of MediaWiki, the software they use. Many other sites
use the same software. IMO it's bad but others may need, so it's
probably good to add something similar in Drupal as long as it's not
the default.
nsk
Previous comments:
------------------------------------------------------------------------
Sat, 11 Jun 2005 20:57:10 +0000 : moshe weitzman
Attached please find a demonstration patch which moves comment display
to its own tab on the node view page. Admin may choose this new view or
old view.
The patch needs work still:
- comment control form redirects to the node page after submit. Should
be comment page
- same for comment posting form
- probably more
I hope someone gets inspired to finish this patch. I won't revisit this
for a while.
------------------------------------------------------------------------
Sat, 11 Jun 2005 21:00:56 +0000 : moshe weitzman
Attachment: http://drupal.org/files/issues/discuss.patch (3.87 KB)
with attachment this time
------------------------------------------------------------------------
Sat, 11 Jun 2005 23:24:12 +0000 : moshe weitzman
oh yeah - also on the TODO list is to comment URLs to always point to
comment.module and never to node/x#comment-cid or node/x#comment-new
and so on. then comment.module can work out whether to show you the
discuss tab or the traditional view.
------------------------------------------------------------------------
Sun, 12 Jun 2005 00:56:02 +0000 : Boris Mann
I like the concept. Would want this to be part of workflow settings, so
that comment-tab could be enabled/disabled per content type.
------------------------------------------------------------------------
Sun, 12 Jun 2005 02:20:42 +0000 : Jaza
I like the idea - big +1. This is a must for sites that have long
articles and/or many comments - viewing the node is enough bandwidth as
it is, without all the comments getting displayed as well. Also improves
usability, IMO.
Would also like to see pagination of the comments tab (I'm sure you've
thought of this already, Moshe). Something similar to A List Apart
[1]'s system, e.g. node/x/discuss/2 (page 2 of that node's comments).
Or, if the existing pager system demands it, node/x/discuss?page=2.
[1] http://www.alistapart.com/
------------------------------------------------------------------------
Tue, 30 Aug 2005 00:41:42 +0000 : chx
Attachment: http://drupal.org/files/issues/discuss_0.patch (3.87 KB)
Let's discuss! Like do we want to have a reply form on the node display
even if the comments are on the discuss page?
------------------------------------------------------------------------
Tue, 30 Aug 2005 00:50:13 +0000 : chx
Attachment: http://drupal.org/files/issues/discuss_1.patch (3.58 KB)
I have uploaded moshe's patch. Let's try again.
------------------------------------------------------------------------
Tue, 30 Aug 2005 11:22:18 +0000 : Bèr Kessels
I like this approach a lot. However, as i use drupal also for blogging,
would dislike my bog to behave the same. Thus there must be some -easy-
way to get around this. µ
The problem lies mainly in the fact that tabs are not administratable
in Drupals menu admin. And I think it is a very bad idea to add a
config option where to place the comments. We should go for one way,
teh default way, and offer a menu-admin of some sort to change this,
for bloggers and slashdot alikes.
How to achieve this? I see two options, but I really hope people see
more then one.:
- make local-tasks appear in the menu admin (gets a huge +1 from me)
It's a separate issue, though.
- add a config option to the comment admin to allow comments either as
tab or as thread under the post. IMO that config option should not set
a variable, that gets checked every hook_menu call, but it should alter
the menu-entry in the database for the tabs Its possible, I did that
before.
But, as said, I hope others have better ideas. for I really would love
this feature to get in. But I am afraid a huge lot of people will move
away from drupal if we allow tabs only.
------------------------------------------------------------------------
Tue, 30 Aug 2005 14:05:59 +0000 : chx
No, not tabs only. I know the drupal_gotos are giving you this
impression, but will fix in the evening. Also, there is only a
variable_get for node/nid pages and then again, variable_get is double
cached, it's very fast.
------------------------------------------------------------------------
Tue, 30 Aug 2005 16:19:22 +0000 : Bèr Kessels
I know variable is fast, but I was referring to:
&& variable_get('comment_task', FALSE) that is evaluated every node
view.
As well as the setting in the comment options. I am no fan of such
options; However, it seems in this case that setting is sortof
required, unfortunately. So unless others come up with better ideas,
just ignore my previous comment/post.
Another idea: put this in hook_nodeapi() $op == "settings". that way
this setting can be used per type. I can imagine that this makes no
sense for forums and blog entries, yet is very usefull for the books on
that same site.
------------------------------------------------------------------------
Tue, 30 Aug 2005 16:20:36 +0000 : Bèr Kessels
sorry, with "Another idea: put this in..." I meant to say "Another idea:
put the settings for tabs vs threads ..."
------------------------------------------------------------------------
Mon, 12 Sep 2005 22:24:22 +0000 : chx
Attachment: http://drupal.org/files/issues/discuss_2.patch (6.71 KB)
Comment URL is introduced. Instead of node/$nid#comment-cid we have
comment/$nid#comment-cid
Other minor fixes.
------------------------------------------------------------------------
Tue, 13 Sep 2005 00:31:31 +0000 : Boris Mann
Comment URLs are great, except I'd love to have a *full* comment URL --
e.g. comment/$nid/$cid. The probably with anchor tags is that they are
not visible at the server level -- i.e. not tracked in referrers, etc.
etc.
Don't want to hijack this issue, so feel free to ignore for now, but
wanted to bring up the issue from a search/web traffic/etc.
perspective.
------------------------------------------------------------------------
Tue, 13 Sep 2005 05:51:09 +0000 : nedjo
+1 in principle. (I've looked over but not applied the code.)
(Optional) tabbed display is a useful enhancement and justifies the
extra configuration option. I also like the new comment URL.
1
0