Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

added estonian and new swedish codes #54

Merged
merged 0 commits into from
Dec 11, 2014
Merged

added estonian and new swedish codes #54

merged 0 commits into from
Dec 11, 2014

Conversation

konstantinsp
Copy link
Contributor

No description provided.

@konstantinsp konstantinsp reopened this Nov 27, 2014
@konstantinsp
Copy link
Contributor Author

added function to translate russian characters to latin characters.
added //IGNORE to deal with umlaut lettrers(tested on finnish umlauts)

@konstantinsp
Copy link
Contributor Author

changed tagging bundle route names to lower case,
besause linux systems are case sensitive and in admin list bundle route names are generated in lower case. this throws error that route path not found.

@konstantinsp
Copy link
Contributor Author

previous getDefaultOptions method does not get executed (in symfony > 2.3?)
setDefaultOptions method sets default options to the field
which fixes this issue: #55

@roderik
Copy link
Contributor

roderik commented Dec 11, 2014

Hi @konstantinsp, thanks for all these fixes!

I would like to get them merged but there are some issues:

  • could you please open one pull request per issue you fix in the future? This way we have a clear view on what is changed and for what reason. This also allows us to merge some, and not others where we feel there is some more work or testing needed.
  • you are using a static method for rus2translit, could you make this "not static"?
  • could you rebase or merge in master to your branch? there is a merge conflict that prevents me from merging it.

jockri
jockri reviewed Dec 11, 2014
View reviewed changes
src/Kunstmaan/TaggingBundle/Form/TagsAdminType.php
@@ -24,6 +24,11 @@ public function buildForm(FormBuilderInterface $builder, array $options)
$builder->addViewTransformer($transformer, true);
}

public function setDefaultOptions(OptionsResolverInterface $resolver)
{
$resolver->setDefaults($this->getDefaultOptions([]));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use the old array syntax? The CMS requires php 5.3.9 or up.

@konstantinsp
Copy link
Contributor Author

could you advice me how to open one pull request per issue?

@roderik
Copy link
Contributor

roderik commented Dec 11, 2014

There are multiple ways, but i do as follows:

  • create a branch in my fork for each thing that i want to fix
  • send a pull request from that branch to the source repo
  • merge my branch into my forks master branch so i have everything in one branch to use until the source repo has merged my request

@konstantinsp konstantinsp force-pushed the master branch 2 times, most recently from 53aa487 to fa3619c Compare December 11, 2014 18:36
@roderik roderik merged commit fa3619c into Kunstmaan:master Dec 11, 2014
@roderik roderik modified the milestone: Q4 2014 - v3.0 Dec 27, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Priority: Normal Target audience: Developers Type: Feature
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@konstantinsp @roderik @jockri