Fixes #36050 - DiffRadioButtons PF4 Refactor #9615

LiorKGOW · 2023-02-08T15:26:47Z

Changing the ButtonGroup to a PF4 ToggleGroup
Changing the name of the component DiffRadioButtons to DiffToggle
Refactoring DiffContainer component from being a class component to be a functional component
Changing the div id wrapping the ButtonGroup to match the name of the new component, and assigning it to the ToggleGroup instead, while removing the unnecessary div
Updating DiffContainer test
Updating the appropriate Snapshots
Keeping used functionalities

Screenshots:

theforeman-bot · 2023-02-08T15:26:50Z

Can one of the admins verify this patch?

theforeman-bot · 2023-02-08T15:26:50Z

Can one of the admins verify this patch?

theforeman-bot · 2023-02-08T15:26:50Z

Can one of the admins verify this patch?

theforeman-bot · 2023-02-08T15:26:52Z

Issues: #36050

LiorKGOW · 2023-02-08T15:29:26Z

@MariaAga @Ron-Lavi
What do you think?

MariaAga

Thank Lior, works well!
needs some css adjustments so there will be equal space between the toggle group, the divider, and the revert button.
We're trying to move away from snapshot testing, can you make a small react testing library test for this instead?

webpack/assets/javascripts/react_app/components/DiffView/DiffToggle.js

webpack/assets/javascripts/react_app/components/DiffView/diffview.scss

LiorKGOW · 2023-02-12T08:35:09Z

Thank Lior, works well! needs some css adjustments so there will be equal space between the toggle group, the divider, and the revert button. We're trying to move away from snapshot testing, can you make a small react testing library test for this instead?

Sure, I'll update you when it's ready 👍🏼
Thank you @MariaAga 🙏🏼

LiorKGOW · 2023-02-12T08:56:44Z

Thank Lior, works well! needs some css adjustments so there will be equal space between the toggle group, the divider, and the revert button. We're trying to move away from snapshot testing, can you make a small react testing library test for this instead?

Regarding the css adjustments you mentioned, I think now there is an equal space between the elements, wouldn't mind another eye taking a look 😶‍🌫️
WYT? @MariaAga

LiorKGOW · 2023-02-12T11:17:54Z

Thank Lior, works well! needs some css adjustments so there will be equal space between the toggle group, the divider, and the revert button. We're trying to move away from snapshot testing, can you make a small react testing library test for this instead?

@MariaAga I am working on the DiffContainer test (which broke because it used enzime).
Since DiffContainer already contains DiffToggle, Do you think I should create another test for DiffToggle,
eg. a wrapper component, with a useState, which returns DiffToggle ?
I was wandering since DiffContainer is doing exactly the same

UPDATE
I have changed the test in DiffContainer to also check the buttons of DiffToggle

webpack/assets/javascripts/react_app/components/DiffView/DiffConsts.js

MariaAga

Tested in report_templates and audits and it looks good, just needs a few code changes

MariaAga · 2023-02-20T20:49:45Z

webpack/assets/javascripts/react_app/components/DiffView/DiffContainer.js

-    this.setState({ viewType });
-  }
+const DiffContainer = ({ patch, oldText, newText, className }) => {
+  const [viewType, setViewType] = useState(SPLIT);


Nice refactor!

Thank you @MariaAga :)

webpack/assets/javascripts/react_app/components/DiffView/DiffContainer.test.js

MariaAga · 2023-02-20T20:52:00Z

webpack/assets/javascripts/react_app/components/DiffView/DiffContainer.test.js

+
+
+  // unmnount the component from the DOM
+  unmount();


Thanks 🙏🏼

package.json

LiorKGOW · 2023-03-06T14:44:53Z

Removed any unrelated snapshot updates 👍🏼
Hopefully I have fixed all of your comments @MariaAga 🙏🏼
I think it is ready to merge

MariaAga

Thanks for the refactor!

theforeman-bot added Not yet reviewed UI labels Feb 8, 2023

LiorKGOW force-pushed the DiffView-DiffToggleButtons-PF4 branch 4 times, most recently from f20e008 to ef969fb Compare February 9, 2023 07:58

MariaAga self-assigned this Feb 9, 2023

LiorKGOW force-pushed the DiffView-DiffToggleButtons-PF4 branch from ef969fb to e7117ce Compare February 9, 2023 13:27

MariaAga requested changes Feb 10, 2023

View reviewed changes

webpack/assets/javascripts/react_app/components/DiffView/DiffToggle.js Outdated Show resolved Hide resolved

webpack/assets/javascripts/react_app/components/DiffView/diffview.scss Outdated Show resolved Hide resolved

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Feb 10, 2023

LiorKGOW force-pushed the DiffView-DiffToggleButtons-PF4 branch from e7117ce to d740f6e Compare February 12, 2023 08:44

theforeman-bot added Needs re-review and removed Waiting on contributor labels Feb 12, 2023

LiorKGOW force-pushed the DiffView-DiffToggleButtons-PF4 branch from d740f6e to 7c9e5e0 Compare February 12, 2023 08:52

LiorKGOW force-pushed the DiffView-DiffToggleButtons-PF4 branch from 7c9e5e0 to 3774bf5 Compare February 12, 2023 09:02

LiorKGOW force-pushed the DiffView-DiffToggleButtons-PF4 branch 2 times, most recently from 7cb6a22 to b801c3a Compare February 12, 2023 15:36

LiorKGOW requested a review from MariaAga February 12, 2023 15:37

LiorKGOW force-pushed the DiffView-DiffToggleButtons-PF4 branch 3 times, most recently from 76a0bc5 to 7ed2bdb Compare February 14, 2023 12:28

MariaAga reviewed Feb 20, 2023

View reviewed changes

webpack/assets/javascripts/react_app/components/DiffView/DiffConsts.js Outdated Show resolved Hide resolved

MariaAga requested changes Feb 20, 2023

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Feb 20, 2023

LiorKGOW force-pushed the DiffView-DiffToggleButtons-PF4 branch from 7d2c9d3 to 4f6bac0 Compare February 21, 2023 09:08

theforeman-bot added Needs re-review and removed Waiting on contributor labels Feb 21, 2023

LiorKGOW force-pushed the DiffView-DiffToggleButtons-PF4 branch 6 times, most recently from 00367f2 to 1a0a440 Compare February 21, 2023 13:16

LiorKGOW force-pushed the DiffView-DiffToggleButtons-PF4 branch from 1a0a440 to ae40b7b Compare March 5, 2023 13:03

LiorKGOW requested a review from a team as a code owner March 5, 2023 16:19

theforeman-bot added the Waiting on contributor label Mar 5, 2023

ekohl reviewed Mar 5, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

LiorKGOW marked this pull request as draft March 6, 2023 12:50

LiorKGOW force-pushed the DiffView-DiffToggleButtons-PF4 branch from 4e31ce7 to ae40b7b Compare March 6, 2023 14:15

theforeman-bot removed the Waiting on contributor label Mar 6, 2023

LiorKGOW force-pushed the DiffView-DiffToggleButtons-PF4 branch from ae40b7b to c6e4048 Compare March 6, 2023 14:24

Fixes #36050 - DiffRadioButtons PF4 Refactor

c208b36

LiorKGOW force-pushed the DiffView-DiffToggleButtons-PF4 branch from c6e4048 to c208b36 Compare March 6, 2023 14:31

LiorKGOW requested a review from MariaAga March 6, 2023 14:41

LiorKGOW marked this pull request as ready for review March 6, 2023 14:42

MariaAga approved these changes Mar 9, 2023

View reviewed changes

theforeman-bot removed the Needs re-review label Mar 9, 2023

MariaAga merged commit 51172da into theforeman:develop Mar 9, 2023
8 checks passed

LiorKGOW deleted the DiffView-DiffToggleButtons-PF4 branch March 12, 2023 10:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #36050 - DiffRadioButtons PF4 Refactor #9615

Fixes #36050 - DiffRadioButtons PF4 Refactor #9615

LiorKGOW commented Feb 8, 2023 •

edited

theforeman-bot commented Feb 8, 2023

theforeman-bot commented Feb 8, 2023

theforeman-bot commented Feb 8, 2023

theforeman-bot commented Feb 8, 2023

LiorKGOW commented Feb 8, 2023 •

edited

MariaAga left a comment

LiorKGOW commented Feb 12, 2023

LiorKGOW commented Feb 12, 2023

LiorKGOW commented Feb 12, 2023 •

edited

MariaAga left a comment

MariaAga Feb 20, 2023

LiorKGOW Feb 21, 2023

MariaAga Feb 20, 2023

LiorKGOW Feb 21, 2023

LiorKGOW commented Mar 6, 2023 •

edited

MariaAga left a comment

Fixes #36050 - DiffRadioButtons PF4 Refactor #9615

Fixes #36050 - DiffRadioButtons PF4 Refactor #9615

Conversation

LiorKGOW commented Feb 8, 2023 • edited

theforeman-bot commented Feb 8, 2023

theforeman-bot commented Feb 8, 2023

theforeman-bot commented Feb 8, 2023

theforeman-bot commented Feb 8, 2023

LiorKGOW commented Feb 8, 2023 • edited

MariaAga left a comment

Choose a reason for hiding this comment

LiorKGOW commented Feb 12, 2023

LiorKGOW commented Feb 12, 2023

LiorKGOW commented Feb 12, 2023 • edited

MariaAga left a comment

Choose a reason for hiding this comment

MariaAga Feb 20, 2023

Choose a reason for hiding this comment

LiorKGOW Feb 21, 2023

Choose a reason for hiding this comment

MariaAga Feb 20, 2023

Choose a reason for hiding this comment

LiorKGOW Feb 21, 2023

Choose a reason for hiding this comment

LiorKGOW commented Mar 6, 2023 • edited

MariaAga left a comment

Choose a reason for hiding this comment

LiorKGOW commented Feb 8, 2023 •

edited

LiorKGOW commented Feb 8, 2023 •

edited

LiorKGOW commented Feb 12, 2023 •

edited

LiorKGOW commented Mar 6, 2023 •

edited