Event color#19
Conversation
|
Will you merge this pull request? |
There was a problem hiding this comment.
why adding color to attr_accessor list when you are proposing it to be a column of the table?
There was a problem hiding this comment.
I'm sorry, but I'm not sure what you mean. I would like the user to be able to set the color of an event. To do so, shouldn't the color be accessible through attr_accessor?
There was a problem hiding this comment.
I am saying that you don't need to put it in the accessor list. It will always be accessible.
There was a problem hiding this comment.
Okey. I'm pretty new to Rails. Would you mind telling me what's the difference from the title and the description that's in the attr_accessor?
There was a problem hiding this comment.
title and description are added as accessors in event_series model so that they can be used to set the values for any event model using that value. See here
Since I don't see that you are setting the color of the event any where in the code, that's why I asked you to remove it.
There was a problem hiding this comment.
So I simply add it like this?
self.events.create(
:title => title,
:description => description,
:all_day => all_day,
:starttime => new_start_time,
:endtime => new_end_time,
:color => color
)
There was a problem hiding this comment.
Yes and add the color picker as input field in the form as I asked in one of my previous comments.
There was a problem hiding this comment.
Do you know where to find such a color picker? How should you choose what color to pick?
There was a problem hiding this comment.
Simply add f.color_field :name to the event form and you are done.
|
hi @ehannes where do we stand on this? |
|
Hi! |
|
That is what I've been looking for. What about adding user_id? User_id + color will be awesome. Can you please write a full project (Fullcalendar_engine + project) so that I can understand how values of color and user_id are added to MySQL. And how to retrieve calendar based on user_id + color. |
|
This pull request will unfortunately not, if I manage to finish it, include connection to user_id. It will only include the possibility to set the color of specific events in the calendar, a feature already available in Fullcalendar. Include user_id is, in my opinion, a totally different topic. |
|
Hannes, I totally agree with you that user_id is a different topic. I see that the color attribute is added to the gem. When i create a new project and Gemfile has that calendar gem and add a new model for user. How can the color of a speciic user be added to the event calendar? |
|
@aditya-kapoor: Took me nearly a year or so, but now I've added the color attribute to the strong parameters list and a color picker to the GUI :) Are we ready to merge now? |
|
@kuldeepaggarwal would you mind checking this PR as well? |
| '#3B91AD' #default color | ||
| else | ||
| event.color | ||
| end |
There was a problem hiding this comment.
- can we extract default color to configuration, so that anyone can change the default as per their need?
- a little code picking:
def set_event_color(event)
event.color.presence || FullcalendarEngine.config.default_color
endThere was a problem hiding this comment.
Where would I include the default value? I had a look in config/initializers/configuration.rb, but if I understand it correctly, those values are passed to FullCalendar. The default_color value should not be passed to FullCalendar, right?
There was a problem hiding this comment.
User should be able to create an initializer file, like devise.rb if you are aware about that. And he can set default values in that configuration file.
|
@ehannes Any updates? |
|
@kuldeepaggarwal Not yet I'm afraid. I'm not sure how to implement the initializer file. Any links or tips about that? |
|
@ehannes We will implement that later, but could you please extract that logic to |
|
We are using this gem for a project and we really want to use this feature. @ehannes do you have any idea when you might be finished? Or is there anything I can help with? |
|
Hi! Sorry for the delay. @kuldeepaggarwal I gave it a try in 4bcc011. Was that what you meant? If not, please feel free to give further instructions. Otherwise I can push the branch to your repo so you can fix it. @scout208 If we just get the default value for event color correct I think it's ready for merge. |
| 'selectable' => true, | ||
| 'timeFormat' => "h:mm t{ - h:mm t}" | ||
| 'timeFormat' => "h:mm t{ - h:mm t}", | ||
| 'defaultColor' => '#3B91AD' |
There was a problem hiding this comment.
rename it to default_color.
There was a problem hiding this comment.
All other variables in this config uses camelCase. Why should it use snake_case?
There was a problem hiding this comment.
because all other variables are getting used on frontend, but if you just noticed at Line no: 18/19 we are using snake_case, as mount_path and default_color will be ruby realted variables.
I will think for a better implementation later.
There was a problem hiding this comment.
Thanks for that clarification! I'll rename it then :)
|
@ehannes Just fix above issues and then I think, we are good to go. |
According to Ruby best practice, setter methods should not be prefixed with 'set'.
Ruby related variables should use snake_case instead of camelCase.
|
I should probably add that I have not tested these changes. And I see that the test coverage of this engine is about 0%... I am using the engine in one of my own applications, but I haven't tested this specific branch. |
|
@ehannes Can you please test this in your app? It will be a great help. |
|
@kuldeepaggarwal Ok, I'll test it. Though, it would be advisable to add at least some basic tests to this engine. |
|
I agree that there must be test cases for the engine. Will plan something for this. Can you please create an issue for this on the repo? |
Adds support for changing color of an event. An attribute "color" is added to the FullcalendarEngine::Event model. Fullcalendar already has this function, and this pull request enables it in this rails engine.
This pull request fixes #18