Posted by mdowsett on September 10, 2008 at 1:08pm
| Project: | Availability Calendars |
| Version: | 6.x-2.x-dev |
| Component: | User interface |
| Category: | feature request |
| Priority: | normal |
| Assigned: | fietserwin |
| Status: | closed (fixed) |
Issue Summary
It'd be nice to be able to set the colors of the statuses in the UI...I had changed it the css file and then upgraded and of course lost those changes.
Great module!
Comments
#1
I'm planning to make the statuses customisable so you could have more than just available, not available and maybe available. With each of those you could specify a colour, or at least a style to use from a stylesheet (haven't thought that through fully yet).
We'd also build the legend from that list. Going to close #306464: legend and merge it into this one.
#2
Legend has been fixed in 6.x using latest dev, I will not be backporting. Will work on configurable states.
#3
Where are you on this? This is another job that I need to implement for a client. Since it's a paid job, I can work on this task right away, unless you've got a reason not to.
I would like to tie this into the work I'm proposing to do on #331151: Ability to use javascript to select availability dates? as well.
#4
that would be great if you have time. I have been too busy to look at this.
#5
Great. I will start working on a patch this week.
#6
Here's a patch to add customizable labels to Availability Calendars.
If you don't set up custom labels, the default labels are used, as before.
Custom labels can be styled using CSS classes derived from the names of the labels.
You can see the generated style for your label in the administration form, but it is simply str_replace(" ", "-", strtolower($label)).
Ex. "My Label" => "my-label"
Default labels keep the CSS classes they have always had.
A word for production sites that wish to use custom labels.
In your database, you will have days marked as follows:
0 => 'Available'
1 => 'Fully booked'
2 => 'Provisionally booked'
In order to ensure a smooth transition, make your custom label ids match up with your existing data. So, if you want to create a custom label for days marked "Fully Booked", create that label before creating any others. Create the "Provisionally booked" replacement label next. Finally, create the "Available" substitute and set that label as the default.
For fresh installs, you don't need to worry about this.
New requirement
In order to make the interface nice, I have used the AHAH Helper module, so you must download and install that module.
#7
This doesn't seem to be patched against latest dev, any way to correct this? Here are the rejects I got:
BTW, this is a pretty extensive change, so will certainly require some additional testing before we commit it, but I definitely like the concept.
#8
Sorry about that.
Here's an updated patch.
#9
I will test the patch today, hopefully others will join.
As for the original post:
The css issue can be overcome by making a copy of the "offending css" in your theme's stylesheet or other stylesheet not ever being changed on upgrades and changing it there. I don't believe that an interface for this is really warranted, but others please give opinions if you like.
#10
You are absolutely right that the correct method is to over-ride the default css settings in your own theme.
The patched version encourages exactly this behavior by creating classes that correspond to the user defined labels, which then must necessarily be styled in the theme stylesheet.
#11
Here are my few comments and requests regarding the patch...
Aside from those, I think this is a very worthy patch and the dependency on ahah_helper should assist in adding javascript selection of the days' statuses.
I hope we can get a few other testers on this. I tested on the checked out version of the repo from cvs and only 1 hunk failed and that was in the info file, since the "drupal added info" wasn't there. I then added the ahah_helper dependency manually.
Thanks for the great work and let me know if you can get to the first two changes or not.
#12
I followed the Drupal convention of having the default hard wired and the overrides in the database. For instance, if you don't declare any regions in your theme info file, you "magically" have the standard Header, Footer, Content, etc. regions available to your theme, even though they're not declared anywhere.
Likewise, I thought that rather than put default labels in the database, they should remain in the code, so that only overrides appear there. Further, as in Drupal themes and other places, once you override one label, you override them all. I think this makes sense and is something Drupal users will be used to.
As for the custom labels, the CSS classes are derived from the labels themselves, so the user is free to impose any sort of convention they choose, without it being restricted by the module. If I have a label called "Honeymoon Suite Booked" and another "Honeymoon Suite Available", I know my css classes will be honeymoon-suite-booked and honeymoon-suite-available. Having the module add "cal" to the front of that seems extraneous to me.
What do you think?
I agree with you that the third item is another issue altogether and deserves it's own ticket.
#13
I follow you on the first item, and agree.
For the second, I still think that something to denote that it is part of the availability_calendar module is important. Perhaps we could add an additional class instead of adding cal to the beginning...
So:
class="availability_calendar_state honeymoon-suite-available"
That way one can easily style any availability_calendar_state in addition to being able to style a specific state.
Lastly, I think maybe we should use:
/*** Converts a string to a suitable html ID attribute.
*
* http://www.w3.org/TR/html4/struct/global.html#h-7.5.2 specifies what makes a
* valid ID attribute in HTML. This function:
*
* - Ensure an ID starts with an alpha character by optionally adding an 'id'.
* - Replaces any character except A-Z, numbers, and underscores with dashes.
* - Converts entire string to lowercase.
*
* @param $string
* The string
* @return
* The converted string
*/
function availability_calendars_id_safe($string) {
// Replace with dashes anything that isn't A-Z, numbers, dashes, or underscores.
$string = strtolower(preg_replace('/[^a-zA-Z0-9_-]+/', '-', $string));
// If the first character is not a-z, add 'n' in front.
if (!ctype_lower($string{0})) { // Don't use ctype_alpha since its locale aware.
$string = 'id' . $string;
}
return $string;
}
for class conversion.
Thanks for your reviewing.
#14
Great -- I absolutely agree with the standards compliant id.
I also agree that adding a generic availability calendars class may be useful.
Will you add that to the code, or should I?
#15
I am attaching a new patch that includes the changes we discussed. I also noticed that your patch seemed to get rid of some new features that were in the dev. Block for key and some other things. I was able to merge the changes in correctly I think.
Please test this new version everyone.
This is patched against the current dev as of today.
#16
#17
Attaching a new version of the patch. The last one had a typo on line 751. $lable instead of $label.
#18
OK, lol. Hopefully this is my last patch on this. I found that doing drupal_add_css within the theme_availability_calendars_node was not enough since the key block also needs this file. So I added it back to init.
Let me know what issues you guys see.
#19
ah what the heck, might as well just add one mo' once...
Change the availability_calendars_option_css to test the strings converted to lowercase. This way one doesn't need to be so precise.
See line #750 after patching to understand.
#20
I am eager to get this feature completed and up as our latest updated release. Anyone had any time for testing/feedback?
#21
On the note of custom colors, I think this could be done by generating stylesheets for the module to add via drupal_add_css(); Then using a color picker or even a simple input field with some validation for hexadecimal color codes.
I will make this into a new issue however.
#22
Sorry, I have been busy this week, but I have downloaded the patch, and plan on testing it tomorrow. I'll let you know how it goes.
Great idea to add a color picker for selecting colors. When you get to that point, I have code in my Theme Hues module that integrates a Javascript color picker and places it in a form field, if you need a reference.
#23
aacraig, thanks but I think I may go the route of http://drupal.org/project/colorpicker and let it degrade down to either no customisation options or degrade to a simple input field if the module is not installed. Then add a notification that if the module is installed then they can use a color picker.
#24
Thanks, I didn't even know that module existed.
#25
aacraig or anyone else... any feedback? I won't be around for about 2 weeks after tomorrow.
#26
Looks good to me.
I got one error when patching:
patching file availability_calendars.csspatching file availability_calendars.info
Hunk #1 FAILED at 2.
1 out of 1 hunk FAILED -- saving rejects to file availability_calendars.info.rej
patching file availability_calendars.install
patching file availability_calendars.module
which is the line regarding the ahah_helper dependency.
I've tested the functionality, though, and everything seems to be working as desired.
#27
the issue is just a problem with the differences in cvs and downloaded versions of the .install
No problem really if this was the only issue you found. I will get this committed this week.
#28
There's an error in the modification form for the calendar, and I'm wondering if it's due to this modification.
availability-calendars/209/2010/04/edit
The current state is not set for the select input, so that when you click save it removes all settings for the form.
#29
hmmm, I am not sure. Could you give me some steps to reproduce it. I can then have a look and see if I can tell what might be causing it.
#30
Also, I have been reading a bit more on ahah in D6 and found this:
http://drupal.org/node/331941
May be worth a look since it wouldn't make ahah_helper a requirement, but it would also require some rewrites.
#31
I just did another project using the ahah_helper. I may try my hand at doing a little rework of this.
#32
I agree that ahah_helper is probably overkill for our purposes, and my use of it was definitely encouraged by my lack of understanding on how the ahah system works.
Now that I have a firmer grasp on it, I would probably not use the helper module.
#33
Re #29
1. Go to any edit form for availability calendar.
2. Insert a reservation date on any day. Save the form.
3. Go back to edit the same week.
Notice that the day you had saved in step 2 is not selected. When you save the form, the previously reserved date will now be deleted.
#34
it would also be nice if you had the possibility of booking half days.
is it possible to add this feature?
#35
I believe there is an open another issue for that. This is an issue completely unrelated.
#36
sorry, i'm new at this. just saw the other open issue...
#37
i encounter the same problem as craig (see #29 / #33)
the module doesn't show the actual status of the days. i think it wil copy the state of the first day of the week to the other days.
it's not that hard to correct it, but you really have to watch out and now which days have what status before you edit a month which already has reservations
#38
Hi,
I just applied the last Patch (n°4) to the 6.x-1.x-dev module under Apache 2.2.14, PHP 5.2.12 and Mysql 5.0.90-community
I get a Table '[database].availability_calendars_labels' doesn't exist on line 701 in availability_calendars.module
I really would like to use this patch...but as I'm new to Drupal and don't know this really great (I mean it) CMS, I don't have time to attempt debugging for the moment :
Is ther a way to quickly fix it ?..when I take a look at availability_calendars.install I can't figure out why it doesn't create that particular table.....
Thanks a lot for your work.
#39
did you run yoursite/update.php ?
#40
Well I just did.....but this error remains...
I first attempted to patch the 6.x-1.2 version with no success....
apparently my tables have all the tfj8 prefix but it seems that this one has not in this call....because there IS a tfj8_availability_calendars_labels in the database...
#41
ok I put brackets around the table name in availability_calendars.module...and I have no more errors.....but now the problem is : when I change a day's disponibility it applies to the entire week....or just don't apply....in fact, it keeps the default....with both php5.2.12 and php5.3...
now I changed
line 389 in availability_calendars.module
while ($daysinweekremaining > 0 && $day <= $month_meta['daysinmonth']) {
$form['week-'. $week]['day-'. $day] = array(
'#type' => 'select',
'#title' => date('l d F', strtotime("$year-$month-$day")),
'#options' => $options,
'#default_value' => $day_status[$day] // instead of $day_state
);
$day++;
$daysinweekremaining--;
}
and it keeps the label...
well the layout relies only on availability_calendars_option_css hardcoded...not an issue for me...
I'm done
Thank you again for this useful tool
#42
Thanks for that Desmatt, I was having that problem as well.
#43
Used patch from
#19on the6.x-1.x-devrelease fromOctober 2010— although I got the same patching errors as#26— I still made the change specified in#41which can be found online 390after patching.Everything seems to be working so far.
Thanks for this awesome module and everyone's work on this issue!!!
#44
Just saw an update on dev and these changes didn't seem to have made it in.
Any idea when this will be rolled into dev?
#45
The missing database table error is because the patch had forgotten to add the Drupal database table prefix.
#46
Patching
#19on6.x-1.x-devrelease from2010-Jul-11errors on patching the.infofile:patching file availability_calendars.infoHunk #1 FAILED at 2.
1 out of 1 hunk FAILED
... So removing the .info file hunk patches with no error. I have attached up updated patch file. Can someone pls review, and commit these changes to dev?
#47
#48
Simply removing the .info portion is not sufficient to get this into the module. Sorry I haven't been around to work on this with you guys. But, there are reported issues with this and the report in #33 of this issue. Please do more testing on this issue to try to see if the issue can be confirmed. I cannot complete such a large code change as well as requiring a new module without being sure this is tested.
Additionally on the note of requiring another module, it seems overkill for a simple administrative settings page. We need to either figure out a way of doing it without the other module or perhaps just say, "Save this page to get more fields"
#49
don't get me wrong, I would like to get this feature into the module... Anyone willing to look over the patch and find the issues would be a big help.
#50
I think the ahah dependency can be removed. I didn't know enough about ahah forms when I wrote the original patch.
Haven't actually been following this thread, so now that I see my patch has issues, I'll see if I can sort them out a put a clean patch up.
Give me 10 days though ;)
#51
aacraig, please be sure to look at the latest 6.x branch. I did some changes to fix other issues. Thanks for taking the time.
#52
#53
Just thought I would say that I just finished one last set of changes. I don't think we should be seeing a moving target anymore for adding in the configurable states. I will be releasing a new version of the module, but the 6.x branch should stay as is until you get this issue sorted. Unless someones site catches on fire ;)
#54
I've rerolled the patch against the latest dev snapshot.
This patch has been reworked by hand to include the latest changes from your current dev snapshot, and I've tested basic functionality. The patch applies fine with the latest dev snapshot.
#55
I have tried the patch aacraig, but it seems it is removing a lot of changes that are in the latest dev. Split days, among other things. Not really sure what is going on with it. I tried starting to merge the changes back in, but got a bit lost in your code.
#56
Oops. That's what I get for just removing chunks of code without reading them first :)
Here's an updated patch which preserves your changes.
#57
actually that still seems to remove the code for splitdays. It also duplicates the code for outputting the calendar key (line 539) instead of using the function availability_calendars_key() like it does currently in dev.
Line 539-560 should likely be changed to:
<?phpif ($settings->showkey === 1) {
$output .= availability_calendars_key();
}
?>
Line 792, where you do options, you have removed the necessary code to output splitday status', here is what it is currently set to in dev:
<?phpfunction availability_calendars_options() {
// TODO: make these configurable
$settings = availability_calendar_getsettings();
$statuses = array(
'available' => t('Available'),
'notavailable' => t('Fully booked'),
'notavailableprov' => t('Provisionally booked'),
);
$ret = array();
foreach ($statuses as $class => $label) {
$ret['cal' . $class] = $label;
}
if ($settings->splitday === 1) {
foreach ($statuses as $class => $label) {
$sub = $statuses;
unset($sub[$class]);
foreach ($sub as $subclass => $sublabel) {
$ret['calsplit cal-' . $class . '_' . $subclass] = t('@a (am)/@b (pm)', array('@a' => $label, '@b' => $sublabel));
}
}
}
return $ret;
}
?>
Thanks for your help on this.
#58
(I'm joining you on this issue, as I'm doing kind of proof of concept for a sales lead which would include this feature as well. So I thought lets test and improve it)
This patch:
- solves the problems mentioned in #57
- solves some other errors (class="1" instead of class="label", take node default instead of system default via call to a..._c..._getsettings())
- some coding standards
- some optimisations (initialize outside loop)
I will do some more testing today or tomorrow, After that, I also hope we can move forward, as communicating changes as large as this is no fun using patch files on moving targets indeed.
1 question: what should be stored in the status field of the availability_calendars_day table? I now see "integer" id's ('1') and multi-word text values ('calsplit cal-1_2'). Is this OK?
#59
Feitserwin, is this patched against the latest DRUPAL-6--1 cvs checkout? It is altering some things that it shouldn't be. Like adding back in t() functions in the install file, as well as other things.
#60
I patched #56 against the available dev version (2011-Jan-23) and started working on that. The t()'s are (still) in the #56 patch and that is how they came in. I did not change that. In a next patch they can be removed (if it is an issue it should be a separate one anyway)
Note : I don't have cvs access (or has everybody read access?), but I assume that the dev version is close enough to the HEAD version.
#61
I started over once more. I’m going to follow the 2nd request of #11 to add the default statuses to the database. To minimize the impact and not requiring an upgrade of the table contents, I’m going to use strings to identify the labels (not integer id’s as used so far). The strings will be the strings that are currently stored in the status field of the day table. Split day statuses remain being assembled in code.
The 3rd request of #11, per node, is definitely a separate issue. The current request turns out to be already hard enough to get committed…
The 1st request of #11, standard css class naming can be added, but I’m thinking about letting the administrator fill this in. A 12-month split day calendar can generate up to 20KB of classes only, by selecting the class names yourself, you can easily save on the page size and avoid collisions at the same time. But, if the class field is not going to be required and left empty, a standard class name can be generated.
To clarify the data model changes, find attached the patch on the install file only. Please code-review them. Comments and requests are welcome.
Continuing now on the code.
#62
Just thought I would say that everyone can do a cvs checkout:
http://drupal.org/project/availability_calendars/cvs-instructions
That is true of all modules on drupal.org. There is a cvs tab on each module's page.
Just checkout, modify, then do a patch:
$ cvs -z6 -d:pserver:anonymous:anonymous@cvs.drupal.org:/cvs/drupal-contrib checkout -d availability_calendars-DRUPAL-6--1 -r DRUPAL-6--1 contributions/modules/availability_calendars/$ cd availability_calendars-DRUPAL-6--1
...[do all your modifications and save changes]...
$ cvs diff -up ../availability_calendars-[issue-number].patch <-- this creates a patch file above the current directory
I will attempt to look at the attached patch today.
#63
A few steps further:
- Install and update as in #61 + weight field
- Possible to edit the states, classes and weights. weight is used for display order in legend and drop-down
- States will be validated (not all labels empty)
- States will be saved (by merely deleting complete contents of table and inserting all states on form)
- Class may be left empty => function from #13 is added to take care of that
TODO:
- Edit calendar form should use database states
- Legend should render database states
- Setting 'default status' should depend on database states
Some notes:
- function from #13: I just always prefix 'cal' to the sanitized state
- hideold is a bit different: we won't know anymore what the "not available"/"booked" state is, so we can't emit that. Instead the stored class will just not be emitted at all. Note that the current code already emits a calpastdate class. That should suffice to render past dates differently. Setting this option thus prevents people - and the tax office :) - from sniffing into your history.
- By storing the default/current classes in the database the theme_availability_calendars_month() function does not need to be changed (except for the hideold and default status). This will lead to a far cleaner patch that will be easier to understand and commit.
- Help text probably is too long. We can shorten that, but for now it gives some extra explanations.
Please code review current code changes. The TODO's will be for tomorrow.
#64
I don't use cvs, and I'll wait for git to incorporate that way of working. Locally I work with Mercurial,so git should be quite easy. But I must say that creating the patches is quite some (error prone) work for me at this moment, so I will definitely switch.
Error prone indeed: file names are probably incorrect in the #63 patch (from .orig to file name as is). So attached a corrected patch.
#65
yes, your patch seems to change the names of the files. Not likely what you intended. Whether you work with cvs locally or not, it is really best to do the patch against the most recent cvs checkout. I don't know what the date for changing to cvs on d.o. is, but it could be later than sooner.
What OS are you on? Installing cvs on mac/linux should be fairly simple and allow a proper checkout.
#66
This patch should be rather complete:
TODO's from #63 are incorporated:
- Edit calendar uses database states
- Legend renders database states
- Setting 'default status' should depend on database states
Some notes:
- The way split classes were derived is not compatible with the new configurable states AND classes. This means a database update for existing split day statuses. We might want to take the opportunity to reduce class length for the existing default states...
- Found a few warnings that will be presented in D7. So I removed them (in using $day_status array on 2 places)
- hideold is a bit different now, see #63, so I changed the help text for that.
- the way defaultstatus is set by the administrator, means that it is no longer possible to set a split state as default. I can't see a use case for that any way, so for me this is no problem. Moreover, at the node level it still is possible, so individual editors can still do so.
- I do not encode labels: the form api does so as well. So, I only encode them when not used in a form element (= in the legend)
- The previous patch introduced an error by making the whole admin settings form a tree. This should have been only the state list part.
Please review and test. Remarks and suggestions are welcome.
#67
Please test this patch and let us know the results? (patch is against the dev version, but that version is the same as the cvs head, I checked that using the instructions from nicholas.alipaz in #62)
I would like to proceed (see [#31060214]), so some reactions and progress would be highly welcomed.
Thanks
#68
fietserwin, thanks for the work and I will be testing as time permits. Likely a little later this week.
#69
This patch are for D6?
#70
Yep, D7 port that includes this patch can be found in #1027930: Port availability calendars to D7.
#71
Committed to the new 6.x-2.x branch. Sorry if this messes up any plans but this is a fairly large rewrite, so we are going to start a new branch.
#72
A little change needed:
As you can see on the picture, there is a problem. The UI for the states is all in one line in my installation using Standard Garland.
In availability_calendars.css change line 20 to
fieldset.state-list .state-item{position:relative;width:100%;clear:left;}(Adding "clear:left;")THX for your work.
EDIT: Just in Case this wasn't clear enough: I downloaded the latest dev from 2011-Mar-05 from the module page
#73
I guess that Garland has no width of itself (fluid), so the width:100% is not enough to force the next item on a new line. I'm currently working on this branch, so I will process it immediately.
thanks for the feedback.
#74
#75
Changed status to fixed:
Already committed to the 6.x-2.x branch a while ago. Reviewing/testing should be done using the 6.x-2.x (or the soon to come 6.x-2.0) release. Bugs and other issues can thus be filed as separate issues against the 6.x.-2.x branch. No need to keep this issue open.
#76
Automatically closed -- issue fixed for 2 weeks with no activity.