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.

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

Add admin UI #227

Merged
merged 9 commits into from Jul 11, 2017
Merged

Add admin UI #227

merged 9 commits into from Jul 11, 2017

Conversation

bmarkons
Copy link
Contributor

@bmarkons bmarkons commented Jul 5, 2017

I used this bootstrap theme :
https://startbootstrap.com/template-overviews/sb-admin-2

I made new admin layout which is used by AdminController and reorganized assets to not have them mixed.

This PR goal is to make manual run possible through admin UI.


def set_repos
@repos = Repo.all
end
Copy link
Member

Choose a reason for hiding this comment

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

Missing blank line after this line :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👌

@repos = Repo.all
end
def set_repo
@repo = Repo.find_by_name(params[:repo_name])
Copy link
Member

Choose a reason for hiding this comment

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

Probably best to use find_by(name: ) here since find_by_* is deprecated in rails 5

@bmarkons bmarkons changed the title Add admin UI [WIP] Add admin UI Jul 7, 2017
@bmarkons
Copy link
Contributor Author

bmarkons commented Jul 7, 2017

Manual run is now possible through Admin UI 🚀

@@ -13,6 +13,7 @@ bundler_args: --without development:production --deployment --retry=3 --jobs=3
before_script:
- bundle exec rake db:create
- bundle exec rake db:migrate
- bundle exec rake assets:precompile &> /dev/null
Copy link
Member

Choose a reason for hiding this comment

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

Hmm this is alittle odd. Why do we need to precompile assets in our test env?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to teampoltergeist/poltergeist#677 (comment), a possible cause of the "Request failed to reach server" problem is that assets are being compiled on-demand, causing enough of a slowdown for PhantomJS to think the connection has failed. This turns off lazy asset compilation in the test environment and precompiles assets before testing on Travis.

I encountered this problem on Travis and solved it with assets precompilation.

Error occurred most probably because of introducing admin layout with its own asset group.

- content_for(:page_header) do
= "#{@repo.name.capitalize} repo"


Copy link
Member

Choose a reason for hiding this comment

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

Style: Alot of blanks lines here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♂️

@@ -0,0 +1,2 @@
- content_for(:page_header) do
= "Dashboard"
Copy link
Member

Choose a reason for hiding this comment

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

One good practice is to always use i18n when displaying text. You never know when we might need to support other languages in the future :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, sure.. forgot about i18n😕 I'll change this 👍

repo = create(:repo)

visit admin_repo_path(repo.name)
page.must_have_content 'Run last'
Copy link
Member

Choose a reason for hiding this comment

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

Using i18n for text that are displayed will also allow you to avoid duplications in the test cases.

@tgxworld
Copy link
Member

OK 👍 LGTM merge away and we can test it out in production :) Good work 👍

@tgxworld tgxworld merged commit 6ae5be8 into ruby-bench:master Jul 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants