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

Convert settings tabs to React #8956

Merged
merged 1 commit into from
Dec 4, 2023
Merged

Convert settings tabs to React #8956

merged 1 commit into from
Dec 4, 2023

Conversation

GilbertCherrie
Copy link
Member

@GilbertCherrie GilbertCherrie commented Nov 6, 2023 •
edited
Loading

Converted Settings > My Settings tabs to React. React tabs are now keyboard accessible.

Before:
Screenshot 2023-11-21 at 3 36 22 PM
Screenshot 2023-11-21 at 3 35 31 PM

After:
Screenshot 2023-11-21 at 3 28 13 PM
Screenshot 2023-11-21 at 3 32 04 PM

@GilbertCherrie GilbertCherrie requested a review from a team as a code owner November 6, 2023 17:30
@miq-bot miq-bot added the wip label Nov 6, 2023
@GilbertCherrie GilbertCherrie force-pushed the fix_settings_tabs branch 3 times, most recently from 3dc47fd to db33510 Compare November 21, 2023 20:28
@GilbertCherrie GilbertCherrie changed the title [WIP] Convert settings tabs to React Convert settings tabs to React Nov 21, 2023
@jeffibm
Copy link
Member

jeffibm commented Nov 22, 2023 •
edited
Loading

Hey @GilbertCherrie , I just noticed a delay in rendering the default tab. It's there in the master branch as well.

I can see 2 apis running in the background.Can we introduce some sort of loading/spinner in here.
Here is how I see-

Screen.Recording.2023-11-22.at.10.26.49.AM.mov

@DavidResende0 , could you also check when you are back?

@GilbertCherrie GilbertCherrie force-pushed the fix_settings_tabs branch 2 times, most recently from 553f2d8 to aa0ac2f Compare November 22, 2023 13:58
jeffibm
jeffibm reviewed Nov 22, 2023
View reviewed changes
app/javascript/components/custom-url-tabs/index.jsx Outdated Show resolved Hide resolved
app/javascript/components/custom-url-tabs/index.jsx Outdated Show resolved Hide resolved
app/javascript/components/custom-url-tabs/index.jsx Outdated
Comment on lines 37 to 42
useEffect(() => {
const tempTabs = [];
tabs.forEach((tab, index) => {
if (tab[0] === currentTab) {
setSelectedTab(parseInt(index, 10));
}

tempTabs.push(
<Tab
key={tab[0]}
id={tab[0]}
className={`${tab[0]}` === currentTab ? 'selected' : 'not-selected'}
label={__(tab[1])}
onClick={() => onTabClick(tab[0])}
/>
);
});
setTabsList(tempTabs);
}, []);
Copy link
Member

Choose a reason for hiding this comment

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

I have a doubt with the setSelectedTab in this useEffect. So this is going to render the component again.

Should we use another useEffect for handling array operation and handle the setTabsList

app/javascript/components/custom-url-tabs/index.jsx Outdated
}, [showConfirm]);

return (
<div>
Copy link
Member

Choose a reason for hiding this comment

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

do we need this div?,
or maybe we can assign a className if its really needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do need it since if we remove it there will be 2 parent elements which are the modal and tablist. This will cause an error so we need to wrap them in a parent div

app/javascript/components/custom-url-tabs/index.jsx Outdated Show resolved Hide resolved
app/javascript/components/custom-url-tabs/index.jsx
if (showConfirm) {
document.getElementsByClassName('selected')[0].classList.remove('bx--tabs--scrollable__nav-item--selected');
} else if (document.getElementsByClassName('selected').length > 0) {
fixTabStyling();
Copy link
Member

Choose a reason for hiding this comment

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

Same here, should we do another useEffect for the showConfirm to handle the fixTabStyling which would also handle the onSecondarySubmit in the <Modal>

app/javascript/components/custom-url-tabs/index.jsx Outdated Show resolved Hide resolved
app/javascript/components/custom-url-tabs/index.jsx Outdated Show resolved Hide resolved
@GilbertCherrie GilbertCherrie force-pushed the fix_settings_tabs branch 3 times, most recently from bce3bfc to bd3fee1 Compare November 22, 2023 14:24
@GilbertCherrie
Copy link
Member Author

To avoid merge conflicts and backport errors lets merge this pr: #8969 first.

@GilbertCherrie GilbertCherrie force-pushed the fix_settings_tabs branch 2 times, most recently from 05edebb to b37c6c9 Compare November 29, 2023 20:29
@GilbertCherrie GilbertCherrie force-pushed the fix_settings_tabs branch 2 times, most recently from 6e2e23c to 4b4f15f Compare November 30, 2023 17:51
@miq-bot
Copy link
Member

miq-bot commented Nov 30, 2023

Checked commit GilbertCherrie@4b4f15f with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
3 files checked, 45 offenses detected

app/controllers/configuration_controller.rb

app/views/layouts/_tabs.html.haml

  • ⚠️ - Line 10 - Hash attribute should end with one space before the closing brace or be on its own line
  • ⚠️ - Line 10 - Hash attribute should start with one space after the opening brace
  • ⚠️ - Line 10 - Line is too long. [142/80]
  • ⚠️ - Line 14 - Hash attribute should end with one space before the closing brace or be on its own line
  • ⚠️ - Line 14 - Hash attribute should start with one space after the opening brace
  • ⚠️ - Line 14 - Line is too long. [136/80]
  • ⚠️ - Line 18 - Performance/CollectionLiteralInLoop: Avoid immutable Array literals in loops. It is better to extract it into a local variable or a constant.
  • ⚠️ - Line 18 - Style/IfInsideElse: Convert if nested inside else to elsif.
  • ⚠️ - Line 18 - Style/PercentLiteralDelimiters: %w-literals should be delimited by [ and ].
  • ⚠️ - Line 19 - Avoid using instance variables in partials views
  • ⚠️ - Line 19 - Performance/CollectionLiteralInLoop: Avoid immutable Array literals in loops. It is better to extract it into a local variable or a constant.
  • ⚠️ - Line 19 - Style/PercentLiteralDelimiters: %w-literals should be delimited by [ and ].
  • ⚠️ - Line 21 - Layout/ArgumentAlignment: Align the arguments of a method call if they span more than one line.
  • ⚠️ - Line 22 - Layout/HashAlignment: Align the keys and values of a hash literal if they span more than one line.
  • ⚠️ - Line 23 - Layout/HashAlignment: Align the keys and values of a hash literal if they span more than one line.
  • ⚠️ - Line 24 - Layout/ArgumentAlignment: Align the arguments of a method call if they span more than one line.
  • ⚠️ - Line 29 - Line is too long. [98/80]
  • ⚠️ - Line 2 - Classes should be listed before IDs (.miq_custom_tab_wrapper should precede #custom-tabs-wrapper)
  • ⚠️ - Line 31 - Avoid using instance variables in partials views
  • ⚠️ - Line 31 - Performance/CollectionLiteralInLoop: Avoid immutable Array literals in loops. It is better to extract it into a local variable or a constant.
  • ⚠️ - Line 31 - Style/IfInsideElse: Convert if nested inside else to elsif.
  • ⚠️ - Line 31 - Style/PercentLiteralDelimiters: %w-literals should be delimited by [ and ].
  • ⚠️ - Line 32 - Avoid using instance variables in partials views
  • ⚠️ - Line 34 - Layout/ArgumentAlignment: Align the arguments of a method call if they span more than one line.
  • ⚠️ - Line 35 - Layout/HashAlignment: Align the keys and values of a hash literal if they span more than one line.
  • ⚠️ - Line 36 - Layout/HashAlignment: Align the keys and values of a hash literal if they span more than one line.
  • ⚠️ - Line 37 - Layout/ArgumentAlignment: Align the arguments of a method call if they span more than one line.
  • ⚠️ - Line 3 - Avoid using instance variables in partials views
  • ⚠️ - Line 43 - Layout/ArgumentAlignment: Align the arguments of a method call if they span more than one line.
  • ⚠️ - Line 44 - Layout/HashAlignment: Align the keys and values of a hash literal if they span more than one line.
  • ⚠️ - Line 45 - Layout/HashAlignment: Align the keys and values of a hash literal if they span more than one line.
  • ⚠️ - Line 46 - Layout/ArgumentAlignment: Align the arguments of a method call if they span more than one line.
  • ⚠️ - Line 4 - Avoid using instance variables in partials views
  • ⚠️ - Line 4 - Line is too long. [133/80]
  • ⚠️ - Line 52 - Line is too long. [143/80]
  • ⚠️ - Line 53 - Hash attribute should end with one space before the closing brace or be on its own line
  • ⚠️ - Line 53 - Hash attribute should start with one space after the opening brace
  • ⚠️ - Line 6 - Avoid using instance variables in partials views
  • ⚠️ - Line 6 - Hash attribute should end with one space before the closing brace or be on its own line
  • ⚠️ - Line 6 - Hash attribute should start with one space after the opening brace
  • ⚠️ - Line 6 - Line is too long. [104/80]
  • ⚠️ - Line 7 - Avoid using instance variables in partials views
  • ⚠️ - Line 8 - Style/RedundantArgument: Argument " " is redundant because it is implied by default.
  • ⚠️ - Line 9 - Avoid using instance variables in partials views

@GilbertCherrie GilbertCherrie force-pushed the fix_settings_tabs branch 5 times, most recently from 0feeea2 to 7b3f54b Compare December 1, 2023 20:51
@jeffibm jeffibm merged commit 1436ef0 into ManageIQ:master Dec 4, 2023
8 checks passed
@GilbertCherrie GilbertCherrie deleted the fix_settings_tabs branch December 4, 2023 13:06
@GilbertCherrie GilbertCherrie mentioned this pull request Dec 4, 2023
Merged
@Fryguy
Copy link
Member

Fryguy commented Jan 5, 2024

Skipping backport to quinteros, because it is already in the branch.

@GilbertCherrie GilbertCherrie mentioned this pull request Jan 16, 2024
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeffibm jeffibm jeffibm approved these changes

Assignees

@jeffibm jeffibm

Labels
accessibility enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@GilbertCherrie @jeffibm @miq-bot @Fryguy