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

Detect pin name clashes #1356

Merged
merged 4 commits into from
Jul 16, 2018
Merged

Conversation

evgenykochetkov
Copy link
Contributor

Closes #1337

Plus a tiny bit of refactoring in xod-client's project selectors.

Copy link
Member

@nkrkv nkrkv left a comment

Choose a reason for hiding this comment

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

Kudos for the refactoring ➕

Minus for attention 😛

It’s important to check normalized pin labels to make sure the scenarios when an input takes an auto-generated name IN3 and manually named output IN3 are not possible.

Try to put an input pulse and an output pulse. Give the output label IN. It should cause the error, but it is not.

* @param {Project} project
* @returns {Either<Error|Patch>}
*/
// TODO: Try to simplify this mess :-D
Copy link
Member

Choose a reason for hiding this comment

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

You’ve simplified the mess 👍 I think this comment should go.

CLASHING_PIN_LABELS: ({ label, pinKeys, trace }) => ({
title: 'Clashing pin names',
note: `${pinKeys.length} pins are named ${label}`,
solution: 'Give pins unique names',
Copy link
Member

Choose a reason for hiding this comment

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

Give the pins

@evgenykochetkov
Copy link
Contributor Author

Done.
But I'm not sure about the result.
screen shot 2018-07-13 at 16 45 02
screen shot 2018-07-13 at 16 45 28
Is this good user experience?

@evgenykochetkov evgenykochetkov force-pushed the fix-1337-detect-pin-name-clashes branch from 1a3d1f4 to a630e79 Compare July 13, 2018 14:06
Copy link
Member

@nkrkv nkrkv left a comment

Choose a reason for hiding this comment

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

Addressed the UX in #1362

What for the essence of this PR I think it is complete 👍

@evgenykochetkov evgenykochetkov force-pushed the fix-1337-detect-pin-name-clashes branch from a630e79 to 4966ff7 Compare July 13, 2018 14:56
@evgenykochetkov evgenykochetkov force-pushed the fix-1337-detect-pin-name-clashes branch from 4966ff7 to 37d3b25 Compare July 16, 2018 09:14
@evgenykochetkov evgenykochetkov merged commit 64b757f into master Jul 16, 2018
@evgenykochetkov evgenykochetkov deleted the fix-1337-detect-pin-name-clashes branch July 16, 2018 09:34
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.

2 participants