Skip to content

Fix for https://github.com/OpenGrok/OpenGrok/issues/1076 #1077

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.

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

Merged
merged 2 commits into from
Mar 24, 2016

Conversation

vbro
Copy link
Contributor

@vbro vbro commented Mar 20, 2016

New feature to allow users to collapse and expand long revision
messages on the history page.

New feature to allow users to collapse and expand long revision
messages on the history page.
@@ -279,8 +279,10 @@ revision2 = revision2 >= hist.getHistoryEntries().size() ? hist.getHistoryEntrie
%><%= author %><%
}
%></td>
<td><a name="<%= rev %>"></a><p><%
<td><a name="<%= rev %>"></a><%
int summaryLength = 200;
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be probably a configuration variable, care to add it?
so people can at least set it using readonly xml config passed to indexer ?

Configurable collapse threshold for revision messages
@vbro
Copy link
Contributor Author

vbro commented Mar 23, 2016

Done. Added a way to configure the threshold. Just curious, are there guidelines for configuration variable names?

@tarzanek
Copy link
Contributor

@vbro not really, but we have too many of them :) ideally we should clean them up
we have your OCA on file, so happily accepting, thank you sir!
fwiw - if you want to document this new variable, feel free to do so in readme, so it's easier for people to set up and don't ask unnecessary questions
(we should probably start a section on such variables that don't have indexer option, but just are taken from read only config in readme, we have quite a lot of them since 0.12 - @tulinkry what do you think?)

@tarzanek tarzanek merged commit 81b51bd into oracle:master Mar 24, 2016
@tarzanek tarzanek added this to the 0.13 milestone Mar 24, 2016
@tarzanek tarzanek self-assigned this Mar 24, 2016
@vbro
Copy link
Contributor Author

vbro commented Mar 24, 2016

The environment variables are well documented in https://github.com/OpenGrok/OpenGrok/blob/master/OpenGrok. But not all the variables in Configuration.java are documented.
Perhaps the readme should have a section on how to setup/configure/customize the application using the read only xml and what all the different config variables mean. I can take a shot at starting such a section.

@kahatlen
Copy link
Contributor

Thanks for this improvement. A couple of suggestions for further improvements:

  • Only the default style sheet is updated. Do we also need to change the polished and offwhite variants? And print.css?
  • The code added to web/utils.js could probably be moved into the domReadyHistory() function. Then it is only executed in the history page.

@tarzanek
Copy link
Contributor

@vbro will you be able to patch the other stylesheets?
Or should we revert for the time being?
(sorry, I completely missed only default theme is patched :( )

@vbro
Copy link
Contributor Author

vbro commented Mar 30, 2016

Yes. I missed that too. Please see #1080.
This includes patching offwhite and polished css files with css rules from another commit.
Would be a good idea to factor out the common css rules to a separate file and have default/offwhite/polished implement specific rules for those styles.

@tarzanek
Copy link
Contributor

@vbro yes, that is a good idea, if you understand css and can do it, it would be wonderful and would simplify new changes, tia!

@kahatlen
Copy link
Contributor

+1. It's a pain to have to remember to update all of them. (If it was up to me, I'd say having one style is enough. Then there's less to test when improving the UI.)

@vladak
Copy link
Member

vladak commented Mar 24, 2017

+1 filed issue #1481 to track the non-default style removal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants