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

[WIP] Convert role form from haml to React and add cypress testing #9248

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

liu-samuel
Copy link
Contributor

@liu-samuel liu-samuel commented Aug 14, 2024

Converts user role form from haml to React and add cypress testing

Old:
Screenshot 2024-08-14 at 2 32 29 PM

New:
Screenshot 2024-08-14 at 2 27 45 PM

Related: ManageIQ/manageiq#23153

@liu-samuel liu-samuel requested a review from a team as a code owner August 14, 2024 18:33
@liu-samuel liu-samuel force-pushed the role-form-conversion branch 3 times, most recently from 5e7b5ea to 435b95f Compare August 14, 2024 19:23
@GilbertCherrie GilbertCherrie self-assigned this Aug 14, 2024
package.json Outdated
@@ -71,6 +72,7 @@
"eonasdan-bootstrap-datetimepicker": "~4.17.49",
"es6-shim": "~0.35.3",
"history": "^4.7.2",
"i": "^0.3.7",
Copy link
Member

Choose a reason for hiding this comment

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

what is i for?

package.json Outdated
@@ -81,12 +83,14 @@
"moment-duration-format": "~2.2.2",
"moment-strftime": "~0.5.0",
"moment-timezone": "~0.5.40",
"npm": "^10.8.2",
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why this is needed either?

@Fryguy
Copy link
Member

Fryguy commented Aug 14, 2024

My only other concern is the checkbox tree may not be carbon-compatible? Even so, it's great there's a react component already out there.

@miq-bot
Copy link
Member

miq-bot commented Aug 19, 2024

Checked commit liu-samuel@0f17df2 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
4 files checked, 11 offenses detected

app/controllers/ops_controller/ops_rbac.rb

app/views/ops/_rbac_role_details.html.haml

  • ⚠️ - Line 10 - Avoid using instance variables in partials views
  • ⚠️ - Line 10 - Line contains trailing whitespace
  • ⚠️ - Line 11 - Layout/ArgumentAlignment: Align the arguments of a method call if they span more than one line.
  • ⚠️ - Line 7 - 3 consecutive Ruby scripts can be merged into a single :ruby filter
  • ⚠️ - Line 8 - Avoid using instance variables in partials views
  • ⚠️ - Line 8 - Line is too long. [96/80]
  • ⚠️ - Line 9 - Avoid using instance variables in partials views

@miq-bot
Copy link
Member

miq-bot commented Sep 6, 2024

This pull request is not mergeable. Please rebase and repush.

@Fryguy Fryguy changed the title Convert role form from haml to React and add cypress testing [WIP] Convert role form from haml to React and add cypress testing Oct 3, 2024
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