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

Fixes #26013 - Add GraphiQL console #6471

Merged
merged 1 commit into from Feb 27, 2019
Merged

Fixes #26013 - Add GraphiQL console #6471

merged 1 commit into from Feb 27, 2019

Conversation

xprazak2
Copy link
Contributor

@xprazak2 xprazak2 commented Feb 7, 2019

Extracted from #5336.

It will be much more useful once we have GraphQL :)

@theforeman-bot
Copy link
Member

Issues: #26013

@timogoebel
Copy link
Member

Lovely, this makes me so happy.
Pro tip: I've been using the electron based graphql on my mac.

@ekohl: Can you ack? We don't need any packaging for this as it's just available in a dev environment.

timogoebel
timogoebel previously approved these changes Feb 7, 2019
View reviewed changes
mmoll
mmoll reviewed Feb 7, 2019
View reviewed changes
bundler.d/graphiql.rb Outdated
@@ -0,0 +1,3 @@
group :development, :test do
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this needed in test?

Copy link
Member

Choose a reason for hiding this comment

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

I guess it's not needed there. Development should be enough. Good catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it from :test

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it should go into development.rb then, as it's in that bundler group.

timogoebel
timogoebel previously approved these changes Feb 8, 2019
View reviewed changes
@lzap
Copy link
Member

lzap commented Feb 13, 2019 •
edited

Was wondering if we want to create some place with all of these helper apps when in development mode. Some tiny icon because this way only graphql people will use that. Let's advertise this on some page. Maybe we have more of these tools available today, the thing is I don't remember.

image

https://triviahappy.com/articles/welcome-to-the-net-every-webpage-from-the-1995-movie-the-net

@iNecas
Copy link
Member

iNecas commented Feb 14, 2019

@lzap what about submenu in navigation bar?

@xprazak2
Copy link
Contributor Author

@lzap, what do you think?

timogoebel
timogoebel reviewed Feb 15, 2019
View reviewed changes
app/registries/menu/loader.rb Outdated

if Rails.env.development?
Manager.map :devel_menu do |menu|
menu.sub_menu :devel_tools, :caption => 'Develop tools', :icon => 'pficon pficon-maintenance' do
Copy link
Member

Choose a reason for hiding this comment

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

I would choose a single word as a caption, e.g. Toolbox.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to Toolbox

@mmoll
Copy link
Contributor

mmoll commented Feb 15, 2019

@xprazak2 tests fail 💔

@timogoebel
Copy link
Member

@xprazak2: Can you rebase, please?

@xprazak2
Copy link
Contributor Author

Rebased.

ekohl
ekohl previously approved these changes Feb 26, 2019
View reviewed changes
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

👍 from a packaging perspective.

@xprazak2
Copy link
Contributor Author

I changed a condition when item is added to menu to Rails.env.development? && defined?(::GraphiQL::Rails::Engine) because tests on Jenkins bundle without development and then call rake...

timogoebel
timogoebel approved these changes Feb 26, 2019
View reviewed changes
Copy link
Member

@timogoebel timogoebel left a comment

Choose a reason for hiding this comment

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

Nice! :-)

@timogoebel
Copy link
Member

@ekohl: Can we please get another ack? Thanks.

ekohl
ekohl previously approved these changes Feb 26, 2019
View reviewed changes
timogoebel
timogoebel requested changes Feb 26, 2019
View reviewed changes
config/routes.rb Outdated
@@ -549,4 +549,8 @@
delete 'group/:group' => 'notification_recipients#destroy_group'
end
end

if Rails.env.development?
Copy link
Member

Choose a reason for hiding this comment

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

This also needs && defined?(::GraphiQL::Rails::Engine)

@mmoll
Copy link
Contributor

mmoll commented Feb 27, 2019

test failures unrelated 💚

@mmoll mmoll merged commit 0a39d23 into theforeman:develop Feb 27, 2019
@mmoll
Copy link
Contributor

mmoll commented Feb 27, 2019

merged, díky @xprazak2!

@lzap
Copy link
Member

lzap commented Feb 28, 2019

Sorry I am late to the party, I love it. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmoll mmoll mmoll approved these changes

@timogoebel timogoebel timogoebel approved these changes

@ekohl ekohl ekohl left review comments

Assignees
No one assigned
Labels
Ready to merge
Projects
None yet
Milestone
No milestone
7 participants
@xprazak2 @theforeman-bot @timogoebel @lzap @iNecas @mmoll @ekohl