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

Reload icon #2386

Merged
merged 47 commits into from Feb 7, 2024
Merged

Reload icon #2386

merged 47 commits into from Feb 7, 2024

Conversation

gabrielgiroe1
Copy link
Collaborator

@gabrielgiroe1 gabrielgiroe1 commented Jan 19, 2024

Description

Fixes #2172

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

Screenshots & recording

Manual review steps

  1. Step 1
  2. Step 2

Manual reviewer: please leave a comment with output from the test if that's the case.

Copy link

codeclimate bot commented Jan 19, 2024

Code Climate has analyzed commit f26b716 and detected 0 issues on this pull request.

View more on Code Climate.

Copy link
Collaborator

@adrianthedev adrianthedev left a comment

Choose a reason for hiding this comment

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

Approach

# API
    field :reviews, as: :has_many, reload_button: do
      current_user.admin?
    end
    field :reviews, as: :has_many, reload_button: true
# in ResourceIndexComponent
<%= render Avo::PanelComponent.new(description: description, data: { component: 'resources-index' }, display_breadcrumbs: @reflection.blank?, reload_button: reload_button) do |c| %>
  • add option on hasBaseField
  • add concern with reload_icon
  • js controller will find its own parent turbo frame
  • if it can't find the parent don't do anything
  • if it finds it, reload it
  • use reload_button: :visible on the field declaration
  • in ResourceIndexComponent we should take this option value and pass it to the PanelComponent
  • in resource index component we have access to the field if it's a reflection field
  • the option should be false by default
  • this option should be available for all has_ fields
  • remove the SVG file
  • use the svg helper to load the SVG

nice to have

  • the option should support the ExecutionContext

app/components/avo/panel_component.html.erb Outdated Show resolved Hide resolved
app/javascript/js/controllers.js Outdated Show resolved Hide resolved
app/components/avo/panel_component.html.erb Outdated Show resolved Hide resolved
Copy link
Collaborator

@adrianthedev adrianthedev left a comment

Choose a reason for hiding this comment

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

Super job!
Thanks @gabrielgiroe1.

I left a few comments to tweak.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We won't need this file once you rebase.
#2453 brought this icon to our repo.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This has not been addressed.
We can remove this file now that we updated heoricons.

app/components/avo/panel_component.html.erb Outdated Show resolved Hide resolved
app/assets/svgs/arrow-path-rounded-square.svg Outdated Show resolved Hide resolved
app/components/avo/panel_component.html.erb Outdated Show resolved Hide resolved
spec/system/avo/reload_button_spec.rb Outdated Show resolved Hide resolved
@adrianthedev
Copy link
Collaborator

adrianthedev commented Feb 7, 2024

let's try this approach for feedback.

  • when clicked, add the animation class animate-spin
  • let's try to reverse the animation.
  • let's use arrow-path

@adrianthedev adrianthedev merged commit ccac08c into avo-hq:main Feb 7, 2024
12 of 18 checks passed
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.

Add reload icon on association fields
3 participants