Conversation
toy
left a comment
toy
left a comment
There was a problem hiding this comment.
I like it a lot, thank you!
As you already told that you are going to move it to a new dev module, I just added few comments, but didn't review completely
|
@toy thanks for the early feedback. After moving it to a module, a lot of your comments are no longer valid, so maybe you can do another review when you have a bit |
|
@klaustopher @cbliard @HDinger do you have any thoughts on this? |
toy
left a comment
toy
left a comment
There was a problem hiding this comment.
Already works great! (after fixing gemfile group) Otherwise a bunch of small improvement suggestions
| end | ||
|
|
||
| group :development do | ||
| gem 'openproject-dev_tools', path: 'modules/dev_tools' |
There was a problem hiding this comment.
As it is internal only gem, Maybe better to specify require: "open_project/dev_tools" to not create the modules/dev_tools/lib/openproject-dev_tools.rb file?
| s.license = "GPL-3.0" | ||
|
|
||
| s.files = Dir["{app,config,lib}/**/*"] | ||
| s.metadata["rubygems_mfa_required"] = "true" |
There was a problem hiding this comment.
Alternative is to disable the rubocop rule # rubocop:disable Gemspec/RequireMFA for the gemspec block
| module OpenProject | ||
| module DevTools | ||
| end | ||
| end |
There was a problem hiding this comment.
No need to create empty modules, as they will be created already in required file
| gem 'ladle' | ||
| end | ||
|
|
||
| group :development do |
There was a problem hiding this comment.
Adding it to development group instead of opf_plugins seem to load it too early (fails to find OpenProject::Plugins) and will not be found as a plugin, maybe better to stop loading in non dev environment in the Engine?
| @@ -0,0 +1,9 @@ | |||
| en: | |||
| dev_tools: | |||
There was a problem hiding this comment.
Looking at few other plugins, we seem to use plugin_openproject_XXX namespace, so plugin_openproject_dev_tools
There was a problem hiding this comment.
I am pretty sure that we don't do this consistently. E.g. storages is not plugin_storages.
There was a problem hiding this comment.
I saw that it was inconsistent, but longer namespace reduces the chance of conflict
| Primer::Alpha::ActionMenu.new( | ||
| classes: "op-app-menu--item", | ||
| menu_id: "op-dev-user-switcher-menu", | ||
| anchor_align: :end |
There was a problem hiding this comment.
anchor_align doesn't seem to be needed, all items are aligned to the end on container level
| button.with_leading_visual_icon(icon: :"person-add") | ||
| button.with_tooltip(text: I18n.t("dev_tools.user_switcher.tooltip")) | ||
|
|
||
| current_user_name |
There was a problem hiding this comment.
Maybe better to leave only the icon, for the item not to change size, and current user can be partially identified by the login icon
| menu.with_group do |group| | ||
| group.with_heading(title: I18n.t("dev_tools.user_switcher.active_users")) |
There was a problem hiding this comment.
As there is only one group, I think it is not needed and items can be added directly on the menu level
| group.with_item( | ||
| href: dev_tools_switch_user_path(user_id: user.id), | ||
| label: user.name, | ||
| content_arguments: { data: { "turbo-method": "post" } } |
There was a problem hiding this comment.
Adding select_variant: :single to Primer::Alpha::ActionMenu.new options and following allow showing current user in the list
| content_arguments: { data: { "turbo-method": "post" } } | |
| content_arguments: { data: { "turbo-method": "post" } }, | |
| active: user == User.current |
| <%= | ||
| render( | ||
| Primer::Alpha::ActionMenu.new( | ||
| classes: "op-app-menu--item", |
There was a problem hiding this comment.
If it is possible to somehow add order: 1 style, this item will be rendered at the end of the flex box
|
I see some dangers was that approach. People tend to copy things that they find and code, so adding half working components for the sake of development as usually risky.. Also I don't know how much sense it makes to invest time into frontend components for things that are never intended to be in the visible. I like the encapsulation in the module however I fear, this will be taken as reference although it shouldn't. What is the use case behind the user switcher component? Maybe it makes sense to discuss that in person (perhaps in the dev open door).. |
I think that's a fair point, thanks. The frontend being put together a bit haphazardly/without actual designs is to save time ofc, but it is true that these will then be visible to any dev searching for references in the codebase to copy. I'll add it to Wednesday's dev open door to get some more opinions 👍🏽 |
|
Closing in favour of opf/openproject-dev_tools#2. Still need to take into account the feedback mentioned here - will do so there |
4 participants
This PR adds a new development environment only
dev_toolsmodule. The intention is that this can be used to store anything that is useful for devs, but shouldn't ever be accessible on prod/shouldn't add to the main code baseFor starters, I added the user switcher since that is something I wanted a few times. Perhaps the custom dev only icons/banner/styles that we currently have can also go in here
TODO