Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I would like to kill this at once.
I did an initial cleanup.
Current UI situation: http://awesomescreenshot.com/0ba6awv12
Overkill of text and options/buttons/textfields.
My suggestion:
views style: http://awesomescreenshot.com/0b96awgdf
views row style: http://awesomescreenshot.com/08a6awr61
- I alrdy fixed the hiding of the date field :D.
- Actually don't like the fieldsets in this context, I don't now if we can have the labels, but not the borders.
- I WANT THE FREAKING DESCRIPTION AFTER MY TITLE :D
Comments? Anyone? :)
Comment | File | Size | Author |
---|---|---|---|
#41 | 1036444-fullcalender-41.patch | 1.39 KB | aspilicious |
#40 | fullcalendar-1036444-40.patch | 21.11 KB | tim.plunkett |
#31 | 1036444-31-fullcalendar-D6.patch | 18.12 KB | aspilicious |
#29 | 1036444-29-fullcalendar-D6.patch | 18.73 KB | aspilicious |
#27 | 1036444-27-fullcalendar-D6-almost-fixed.patch | 18.45 KB | aspilicious |
Comments
Comment #1
tim.plunkettIf we're removing the autodiscovery, then the date field needs to be specified.
Comment #2
aspilicious CreditAttribution: aspilicious commentedI think I'm done...
I changed a lot of code.
Small summary:
- Views UI cleanup
- Remove autodiscovery
- Allow the use of fields
==> for every date field you select the date will be added to the calendar.
NEW POSIBILITIES:
* Split an event in multiple time periods, they all will be added.
* Select only the date fields you would like to see on the calender. == more control of the view
- fixed title/url replace == nize!
TODO:
- Tweak the UI a little more like I said before, can be a new issue.
- If the date field is empty there are errors (also before this patch) ==> Make a new issue?
Comment #3
aspilicious CreditAttribution: aspilicious commentedQuick code review of my own patch.
Now I'm done, other people can review/edit.
This sneaked in and is still wrong, of needs to be removed
We need some kind of check if there are dates to display and check if they are empty :)
The removed code isn't correct anymore, just putting it here for referenc.
I renamed the param in the code
Powered by Dreditor.
Comment #4
aspilicious CreditAttribution: aspilicious commentedFixed all the things in the previous review.
TODO:
Fix update, isn't working anymore... :(
Comment #5
tim.plunkettWe can't use $node->language like that, there are per-field language settings as well.
I want to completely destroy autodiscovery. They should be forced to pick a date field.
Here are other tweaks. Attached is the whole patch, and an innerdiff of this and #4. It doesn't make any real changes.
Comment #6
aspilicious CreditAttribution: aspilicious commented"We can't use $node->language like that, there are per-field language settings as well."
Fixed :D
"I want to completely destroy autodiscovery. They should be forced to pick a date field."
Thats is alrdy done? Or isn't it? Me confused...
I *ONLY* use the fields that are specified in the views UI.
Can you explain a little more what you expect?
Comment #7
aspilicious CreditAttribution: aspilicious commentedThere where some warnings and errors when we didn't select any date field.
Caused by a non initialised array.
Comment #8
aspilicious CreditAttribution: aspilicious commentedNeeds review... I guess?
Comment #9
tim.plunkettWhy not have a custom date selection like the URL and Title, in case two date fields are added?
Views throws errors if no fields are added. Perhaps we should throw an error if no date field is added?
If those two get addressed, this is pretty close to awesome :)
Comment #10
aspilicious CreditAttribution: aspilicious commentedBut what If I would Like to add two date fields for one event. I have use cases for that. I have an event that is split into two "time frames".
AWSOME EVENT
---------------
Date first frame:
Sunday, January 23, 2011 - 13:00 - 19:00
Datum second frame:
Tuesday, January 25, 2011 - 21:56 - Thursday, January 27, 2011 - 21:56
The current implementation supports that and just doesn't show any events when there are none defined. And if you only want one of the two "time frames" displayed, throw the second field out of your field list. Simple as that...
I made 3 different calendars with the same data.
An error is a little hard or not? But maybe we can add a status message above the calendar??
"No date fields are selected"
EXTRA: attached patch fixes the drag and drop saving, base path issue
TODO (just for reference):
When this is fixed we should pay attention to one problem:
1) Apparantly IE7 doens't display the calendar :(
Comment #11
aspilicious CreditAttribution: aspilicious commentedTODO will be fixed by: #980180: Extra trailing comma breaks Fullcalendar in IE 7 & IE 6 [BROKEN AGAIN]
Comment #12
aspilicious CreditAttribution: aspilicious commentedNew version
Comment #13
tim.plunkettCould just be $title_field = $fields[$vars['options']['fullcalendar_title_field']];
No need for the extra assignment. Similar to above.
Same as title section.
Trailing spaces, tabs. And unneeded assignment.
Space between if ()
Document $field_names. Perhaps an inline comment as well
The dash with spaces always bugged me. How about 'Fields (FullCalendar)'?
No reason to rename this. Especially since $date_fields is used below.
Wasn't this just wrapped in t() above?
Powered by Dreditor.
Comment #14
aspilicious CreditAttribution: aspilicious commentedLast reroll? :)
Comment #15
aspilicious CreditAttribution: aspilicious commentedTimplunkett I think you were right. When I remove every node I get this:
Notice: Undefined property: stdClass::$nid in template_preprocess_views_view_node_fullcalendar()
So could you put the code back preventing this issue?
EDIT:
Tested it locally, it works when you put it back.
Comment #16
tim.plunkettAmazing work aspilicious!
I cleaned up some of the code, and I think I fixed the multiselect.
Regular patch is against a clean branch, innerdiff is against code patched with #14.
Comment #17
aspilicious CreditAttribution: aspilicious commentedLOL, you're to fast :(. This morning I woke up and said to myself, stupid idiot you forgot the select process in the multiselect...
I run to my laptop and I see your commit...
:D
I tested a gazillions things and I think we are safe...
I would like to RTBC this but I feel like I'm not the person marking this RTBC.
Oh btw, shouldn't we open a 7.2 branch?
Comment #18
tim.plunkettWe really have the benefit of being in two different timezones, it's like there's always someone hacking on fullcalendar at all times!
Maybe geerlingguy can test it.
And yeah, this would be the first commit to the 7.2 branch.
Comment #19
geerlingguy CreditAttribution: geerlingguy commentedIn my testing, I couldn't select any fields - even after clearing caches. Then, after having cleared said caches, no events showed on my fullcalendar (was working fine before patch).
[Edit: D'oh! I didn't notice that I had to first add the field to the view, then select the field from the select area. Just a matter of documentation, really. We should maybe put a little notice at the top of the Field Settings pane in the View that says "Fields must be added to the view before they are available to select here." (something like that).
But it's up to timplunkett - otherwise, the patch works flawlessly, and I'd RTBC. Either put in the description or not, but it's practically ready for commit.
Comment #20
tim.plunkettCommitted to DRUPAL-7--2.
http://drupal.org/cvs?commit=489546
Needs backport.
Here's the final patch I committed, with string tweaks.
Comment #21
aspilicious CreditAttribution: aspilicious commentedNeeds #1035246: Incompatible with Views 6.x-3.x
Ok first try :) (btw I hate D6)
TODO
-----
- the ctools hiding stuff doesn't work anymore
- disabled custom date field cause it's broken
- had to make 2 extra functions => review
Comment #22
aspilicious CreditAttribution: aspilicious commentedYou broke the custom settings in your last patch. I missed that on my final review.
Here is a D7 followup patch. (found it while working on the D6 stuff)
Comment #23
tim.plunkett#21,
Views doesn't depend on CTools in D6, so we can't expect them to have it. Instead of
'#process' => array('ctools_dependent_process')
, it's'#process' => array('views_process_dependency')
. The #dependency line should be fine.I'll look at the other stuff later (those new functions look unfortunate), but you might as well roll in the option renaming from #22. And mind your tabs!
#22,
Yeah, whoops. Committing.
Comment #24
aspilicious CreditAttribution: aspilicious commentedI need to do something about those tabs...
Can't manage to configure my IDE well enough...
+
Dreditor is broken on latest greasemonkey :(
#22 is alrdy in there.
Lets find those damn tabs...
Comment #25
aspilicious CreditAttribution: aspilicious commentedStill tabs in it
Comment #26
aspilicious CreditAttribution: aspilicious commentedWithout tabs *I hope*.
And now only selected fields will be displayed.
Comment #27
aspilicious CreditAttribution: aspilicious commentedAnother try...
Comment #28
aspilicious CreditAttribution: aspilicious commentedInvestigating an other way to fix this, so we can remove the node load dependency...
Comment #29
aspilicious CreditAttribution: aspilicious commentedOk got some interesting results.
1) I copied the workflow from D7. ==> we don't need the two extra functions anymore! Yeah!
2) I only strip the _value in the end ==> Yihaa!
3) Apparantly the value of the field is attached to the node. It is impossible to fetch the value of a field without loading the node. Or I couldn't find it ==> NOOOOO
==> we are looking for a field_get_items but that doesn't rly. Or does it? :)
Comment #30
aspilicious CreditAttribution: aspilicious commentedwtf thats not the correct patch...
Comment #31
aspilicious CreditAttribution: aspilicious commentedBetter?
Srry for the overload off patches :)
Comment #32
aspilicious CreditAttribution: aspilicious commentedAfter investigation, I think it is safe to give this a serious review. We need some more investigation how to delete the node->load call but that must not prevent this initial patch to be committed. We are not rdy for a stable release so we have still time to search for a solution. But I would like to deal with that in an other issue.
OK?
Comment #33
aspilicious CreditAttribution: aspilicious commentedCan someone give this a review? I would like to port all the other patches but they need a clean views UI :).
Comment #34
corey.aufang CreditAttribution: corey.aufang commentedPatch in #1036444-31: Cleanup the views UI + remove autodiscovery mega issue. breaks at line 37 of views_plugin_node_fullcalendar.inc
The data in field_options does not match what is returned by fullcalendar_date_fields.
You can see whats supposed to match, but the data returned by get_field_labels appends "_value" to cck fields.
Comment #35
aspilicious CreditAttribution: aspilicious commentedIt don't break at my installation...
can you try a new view?
Comment #36
corey.aufang CreditAttribution: corey.aufang commentedI mean it breaks in the sense that when you choose row style "Fields (FullCalendar)" and select "Use a custom date field." The field list is empty.
Comment #37
aspilicious CreditAttribution: aspilicious commented- Read the instructions of version 2: http://drupal.org/node/1056742#usage
1) You NEED to add a field first.
==> There is also a warning on the page: 'Add fields to the view in order to customize fields below.',
2) custom date fields are optional, if you do not select one it will use all the date fields added in the views UI.
Comment #38
corey.aufang CreditAttribution: corey.aufang commentedOpps, I wasn't applying the patch to the dev version. :/
Comment #39
tim.plunkettEven though D6 is boring to me now, I know I should review this.
Actually, someone else should review it, and I should look over it after that.... but yeah, assigning to myself.
Comment #40
tim.plunkettThis merges in #1049028: Remove theme_fullcalendar_link, I couldn't get it to work without that.
Comment #41
aspilicious CreditAttribution: aspilicious commentedDefault date thingie was broken.
A diff fix attached.
Default date starting with week view or day view doesn't give the correct results. It has the same effect in the D7 version (I rechecked) so it isn't caused by this patch. But it is worth mentioning. Maybe we need to convert the day and week days/numbers. But that needs to go into D7 first.
Besides of that I couldn't find any problems... No warnings, no errors, ... (and I test a lot) I suggest pushing this, at least we have a solid bas to work on.
Comment #42
tim.plunkettAwesome, so nice to have this done. Now to backport the gcal stuff...
Oh, and open another issue for that date bug, I guess.
Comment #44
SocialNicheGuru CreditAttribution: SocialNicheGuru commentedthis didn't make it into the 1.4 version but is needed there also.
Comment #45
aspilicious CreditAttribution: aspilicious commentedSrry but this will never be integrated into the 1.4 version. You will need to upgrade if you would like all of this.