Show alert upon closing modal via escape keypress; partially close #2613. #2704

nicolad · 2020-04-17T13:45:07Z

Problem this Pull Request solves

Creates ModalWithAlert in app layer to add alert in error cases
Partially closes: Add Confirmation Modals to Event Handlers for Destructive Processes #2613

How has this been tested

nicolad · 2020-04-17T13:46:19Z

package.json

@@ -74,6 +74,7 @@
 		"date-fns": "^2.11.1",
 		"date-fns-tz": "^1.0.10",
 		"emotion-theming": "^10.0.27",
+		"exenv": "^1.2.2",


Added this since we are already using that indirectly via Chakra: https://github.com/chakra-ui/chakra-ui/blob/master/packages/chakra-ui/package.json#L31

LOLZ I added a function with the exact same name for the toaster
it's likely the exact same logic too, since I think that package is a duplication of internal logic from React

got tricked by chakra: https://github.com/chakra-ui/chakra-ui/blob/master/packages/chakra-ui/src/Modal/index.js#L22

but followed your approach, which is less dependent on external stuff

nicolad · 2020-04-17T13:46:52Z

assets/src/infrastructure/ui/layout/modal/modalCloseButtonProps/styles.scss

+		svg {
+			height: var(--ee-border-radius-big);
+			margin: 0;
+			width: var(--ee-border-radius-big);
+		}


I like this better:

I needed --ee-border-radius-big to match 12px which is the same dimension as in chakra.

Open to better variable names than --ee-border-radius-big

ya I'll most likely tweak things once everything is converted, but that doesn't look bad as is

nicolad · 2020-04-17T13:55:04Z

assets/src/domain/eventEditor/ui/ticketAssignmentsManager/components/ModalContainer.tsx

-		title = sprintf(__('Ticket Assignment Manager for Datetime: %s - %s'), entity.dbId, entity.name);
+		title = sprintf(__('Ticket Assignment Manager for Datetime: %s - %s'), String(entity.dbId), entity.name);
 	} else if (assignmentType === 'forTicket') {
-		title = sprintf(__('Ticket Assignment Manager for Ticket: %s - %s'), entity.dbId, entity.name);
+		title = sprintf(__('Ticket Assignment Manager for Ticket: %s - %s'), String(entity.dbId), entity.name);


crazy... never had that before

I had that many times 😄

nicolad · 2020-04-17T15:00:47Z

Got some tests which are failing only on Travis:

locally:

tn3rb

AWESOME POSSUM

looks great... I did have a couple of minor enhancement requests

tn3rb · 2020-04-17T15:08:53Z

assets/src/application/ui/layout/modal/ModalWithAlert.tsx

+					cancelButton={<ButtonAdapter buttonText={__('No')} ref={cancelRef} onClick={onClose} />}
+					header={__('Are you sure you want to close this modal?')}
+					isOpen={isOpen}
+					leastDestructiveRef={cancelRef}
+					okButton={
+						<ButtonAdapter buttonText={__('Yes')} variantColor='red' onClick={props.onClose} ml={3} />


let's change all of these i18n strings into variables and then also add optional props for them with these as the defaults, ie:

const cancelBtnText = cancelButtonText ? cancelButtonText : __('No'); // etc

As well, let's change the header text to just be:

__('Are you sure you want to close this?')

as most non-devs won't understand what a modal is

as most non-devs won't understand what a modal is

good one, thx

updated this, with a slightly changed approach, but with the same result;
thx for suggestion

tn3rb · 2020-04-17T15:10:20Z

assets/src/domain/eventEditor/ui/ticketAssignmentsManager/components/ModalContainer.tsx

-		title = sprintf(__('Ticket Assignment Manager for Datetime: %s - %s'), entity.dbId, entity.name);
+		title = sprintf(__('Ticket Assignment Manager for Datetime: %s - %s'), String(entity.dbId), entity.name);
 	} else if (assignmentType === 'forTicket') {
-		title = sprintf(__('Ticket Assignment Manager for Ticket: %s - %s'), entity.dbId, entity.name);
+		title = sprintf(__('Ticket Assignment Manager for Ticket: %s - %s'), String(entity.dbId), entity.name);


crazy... never had that before

tn3rb · 2020-04-17T15:12:19Z

assets/src/infrastructure/ui/layout/modal/modalCloseButtonProps/index.tsx

+import './styles.scss';
+
+const modalCloseButtonProps: ButtonProps = {
+	className: 'confirm-close',


plz namespace all class names, ie: ee-confirm-close

thx, updated

tn3rb · 2020-04-17T15:14:01Z

assets/src/infrastructure/ui/layout/modal/modalCloseButtonProps/styles.scss

+		svg {
+			height: var(--ee-border-radius-big);
+			margin: 0;
+			width: var(--ee-border-radius-big);
+		}


ya I'll most likely tweak things once everything is converted, but that doesn't look bad as is

tn3rb · 2020-04-17T15:16:03Z

package.json

@@ -74,6 +74,7 @@
 		"date-fns": "^2.11.1",
 		"date-fns-tz": "^1.0.10",
 		"emotion-theming": "^10.0.27",
+		"exenv": "^1.2.2",


LOLZ I added a function with the exact same name for the toaster
it's likely the exact same logic too, since I think that package is a duplication of internal logic from React

tn3rb

POIFECT 👌

manzoorwanijk · 2020-04-17T16:55:46Z

assets/src/application/ui/layout/modal/ModalWithAlert.tsx

+	useEffect(() => {
+		canUseDOM && document.addEventListener('keydown', onEscape);
+
+		return () => {
+			canUseDOM && document.removeEventListener('keydown', onEscape);
+		};


For clarity, better use if (canUseDOM) {}

I followed the same example as in chakra: https://github.com/chakra-ui/chakra-ui/blob/master/packages/chakra-ui/src/Modal/index.js#L131

but if the clarity is an issue, then it's better to make it more explicit, thx for letting me know this 👍

manzoorwanijk · 2020-04-17T16:57:44Z

assets/src/domain/eventEditor/ui/ticketAssignmentsManager/components/ModalContainer.tsx

@@ -10,9 +10,9 @@ const ModalContainer: React.FC<ModalContainerProps> = ({ isOpen, onClose, ...pro

 	let title = '';
 	if (assignmentType === 'forDate') {
-		title = sprintf(__('Ticket Assignment Manager for Datetime: %s - %s'), entity.dbId, entity.name);
+		title = sprintf(__('Ticket Assignment Manager for Datetime: %s - %s'), String(entity.dbId), entity.name);


Always better to use language constructs (which are faster) instead of calling functions. Although, it won't make a big difference here but still it's better.

Suggested change

title = sprintf(__('Ticket Assignment Manager for Datetime: %s - %s'), String(entity.dbId), entity.name);

title = sprintf(__('Ticket Assignment Manager for Datetime: %s - %s'), `${entity.dbId}`, entity.name);

I would prefer clear and clean code over minor performance optimization.
It's not the case here, but in order to know exactly what is the performance gain or loss, I'd have to measure both cases and see exact number. And, if I'd do that probably there wouldn't be very noticeable difference.

However, let's take aside performance discussion.
I think there is a bit of the problem with sprintf.
As I understand sprintf is returning a string, then why I should care about these type coercions?
I think sprintf should be smart enough to this and maybe other check and I should just pass whatever I want. If I pass undefined or null, ignore it, otherwise do your smart job and please don't bother me with type coercion.

I love the DX classnames have, I can pass whatever I want, it can be null, object or string and then it's smart enough to figure out by itself what needs to be the result.

At some point, maybe, we should create our own wrapper for sprintf and make it a bit smarter.

classnames is awesome:

classNames('foo', 'bar'); // => 'foo bar' classNames('foo', { bar: true }); // => 'foo bar' classNames({ 'foo-bar': true }); // => 'foo-bar' classNames({ 'foo-bar': false }); // => '' classNames({ foo: true }, { bar: true }); // => 'foo bar' classNames({ foo: true, bar: true }); // => 'foo bar' // lots of arguments of various types classNames('foo', { bar: true, duck: false }, 'baz', { quux: true }); // => 'foo bar baz quux' // other falsy values are just ignored classNames(null, false, 'bar', undefined, 0, 1, { baz: null }, ''); // => 'bar 1'

manzoorwanijk · 2020-04-17T17:01:34Z

assets/src/application/ui/layout/modal/ModalWithAlert.tsx

+	const cancelBtnText = props.cancelBtnText || __('No');
+	const header = props.header || __('Are you sure you want to close this?');
+	const okBtnText = props.okBtnText || __('Yes');
+	const onEscape = ({ keyCode }) => keyCode === ESCAPE && onOpen();


Not good for TS, try to set a non void return type of the function to see the problem with that magical shortcut 😄

agree, made it TS-friendly:

const onEscape = ({ keyCode }): void => { if (keyCode === ESCAPE) { onOpen(); } };

was in one-liner mood before 😃

manzoorwanijk

LGTM 👍

nicolad · 2020-04-20T10:24:32Z

assets/ZZZ/components/ui/styles/themes/default/index.js

@@ -1,8 +0,0 @@
-import './color-variables.css';


was getting this error:

we have this in EDTR/refactor-master in case we might need this, no need to keep this here

nicolad · 2020-04-20T10:40:42Z

config/webpack.common.js

-	{
-		entry: {
-			'eventespresso-core-css-default': pathToEDTRv1 + 'components/ui/styles/themes/default/index.js',
-		},
-		module: moduleConfigWithJsAndCssRules,
-		watchOptions: {
-			poll: 1000,
-		},
-	},


attempt #2 for:

nicolad · 2020-04-20T10:41:20Z

assets/src/domain/eventEditor/services/apollo/queries/prices/test/useFetchPrices.test.ts

@@ -38,7 +38,6 @@ describe('useFetchPrices', () => {

 		await waitForNextUpdate({ timeout }); // wait for response

-		expect(result.current.error).toBeDefined();


locally I was getting error because of this line, weird

@manzoorwanijk any ideas about this?

nicolad · 2020-04-20T10:41:56Z

config/webpack.common.js

-	{
-		entry: {
-			'eventespresso-core-css-default': pathToEDTRv1 + 'components/ui/styles/themes/default/index.js',
-		},
-		module: moduleConfigWithJsAndCssRules,
-		watchOptions: {
-			poll: 1000,
-		},
-	},


this file will completely/ safely removed in #2609

nicolad · 2020-04-20T11:12:26Z

package.json

@@ -95,6 +95,7 @@
 	},
 	"devDependencies": {
 		"@apollo/react-testing": "^3.1.4",
+		"@babel/compat-data": "^7.9.0",


Added this temporary, to make this failing pass:

As I wrote in another comment, this non-sense will be removed/ adjusted here: #2609

tn3rb

LGTM 🌮🌮🌮🌮🌮

nicolad added type:user-proofing 🛡 EDTR v2 labels Apr 17, 2020

nicolad added this to the EDTR Sprint 11 : Revenge of the Scripts milestone Apr 17, 2020

nicolad requested review from tn3rb and manzoorwanijk April 17, 2020 13:45

nicolad self-assigned this Apr 17, 2020

nicolad commented Apr 17, 2020

View reviewed changes

nicolad force-pushed the EDTR/confirmation3 branch 3 times, most recently from f1e1011 to 0635953 Compare April 17, 2020 14:43

tn3rb requested changes Apr 17, 2020

View reviewed changes

nicolad force-pushed the EDTR/confirmation3 branch from ecf16fc to 9678923 Compare April 17, 2020 15:27

tn3rb previously approved these changes Apr 17, 2020

View reviewed changes

manzoorwanijk reviewed Apr 17, 2020

View reviewed changes

nicolad dismissed tn3rb’s stale review via 33432a8 April 17, 2020 17:17

manzoorwanijk previously approved these changes Apr 17, 2020

View reviewed changes

nicolad added 4 commits April 20, 2020 12:06

Adjusted closeButton; js_only.

41b5ff8

Conflicts fixed.

7ed6e49

Updated ModalWithAlert; js_only.

1dca460

Updated modalCloseButtonProps; js_only.

9566001

nicolad added 3 commits April 20, 2020 12:11

canUseDOM | js_only

3d59bbf

Updated onEscape; js_only.

84c5bac

Updated ModalWithAlert; js_only.

6548f1f

nicolad dismissed manzoorwanijk’s stale review via 6548f1f April 20, 2020 09:11

nicolad force-pushed the EDTR/confirmation3 branch from e57a1d0 to 6548f1f Compare April 20, 2020 09:11

nicolad commented Apr 20, 2020

View reviewed changes

nicolad force-pushed the EDTR/confirmation3 branch 2 times, most recently from 5bb457c to 440bbe5 Compare April 20, 2020 10:40

nicolad commented Apr 20, 2020

View reviewed changes

nicolad force-pushed the EDTR/confirmation3 branch 2 times, most recently from 5c84d5f to 06c6223 Compare April 20, 2020 11:03

nicolad commented Apr 20, 2020

View reviewed changes

nicolad force-pushed the EDTR/confirmation3 branch from e0d01f5 to 36b06fc Compare April 20, 2020 11:17

Failing tests fix; js_only.

62dff69

nicolad force-pushed the EDTR/confirmation3 branch from 36b06fc to 62dff69 Compare April 20, 2020 12:13

nicolad requested a review from manzoorwanijk April 20, 2020 12:20

tn3rb approved these changes Apr 20, 2020

View reviewed changes

nicolad merged commit 10ed7a0 into EDTR/master Apr 20, 2020

nicolad deleted the EDTR/confirmation3 branch April 20, 2020 17:00

	title = sprintf(__('Ticket Assignment Manager for Datetime: %s - %s'), String(entity.dbId), entity.name);
	title = sprintf(__('Ticket Assignment Manager for Datetime: %s - %s'), `${entity.dbId}`, entity.name);

		@@ -38,7 +38,6 @@ describe('useFetchPrices', () => {

		await waitForNextUpdate({ timeout }); // wait for response

		expect(result.current.error).toBeDefined();

Show alert upon closing modal via escape keypress; partially close #2613. #2704

Show alert upon closing modal via escape keypress; partially close #2613. #2704

Conversation

nicolad commented Apr 17, 2020

Problem this Pull Request solves

How has this been tested

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolad commented Apr 17, 2020

tn3rb left a comment

Choose a reason for hiding this comment

AWESOME POSSUM

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tn3rb left a comment

Choose a reason for hiding this comment

POIFECT 👌

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manzoorwanijk Apr 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manzoorwanijk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tn3rb left a comment

Choose a reason for hiding this comment

LGTM 🌮🌮🌮🌮🌮

manzoorwanijk Apr 17, 2020 •

edited

Loading