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

feat: added some additional linting rules to autofix prettier related… #59

Merged
merged 8 commits into from
Dec 18, 2020

Conversation

DarkPurple141
Copy link
Contributor

PR adds functionality to partially address #30 and #24

I'm sure this can be done better.

… issues, added filesystem based upload download API
package.json Outdated Show resolved Hide resolved
src/panel/top-bar.tsx Outdated Show resolved Hide resolved
@DarkPurple141
Copy link
Contributor Author

@AndrewOCC

re: feedback

I think you're right if we go with this pseudo-selected pattern the icons should be inverted. I've amended that. The accessibility change is a bit meh, I'm not sure it was necessary to begin with in the context of this tool (there are deeper issues in that sense and it's a bit of a rabbit hole we'd rather avoid).

I've amended the aria-label to literally just be the icon name, which is better and probably better complements the button text for screen readers.

Hope that works 👍

@DarkPurple141 DarkPurple141 merged commit ca22500 into master Dec 18, 2020
@DarkPurple141 DarkPurple141 deleted the issue-30-issue-24 branch December 18, 2020 01:24
@@ -9,6 +9,8 @@ export type MachineEvents =
| { type: 'FINISH'; results: TaskGroupResult[] }
| { type: 'PIN' }
| { type: 'UNPIN' }
| { type: 'SAVE' }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we rename this event to SAVE_TO_FILE so that is nicely paired with LOAD_FROM_FILE ? :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep

};
}),
},
LOAD_FROM_FILE: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are effectively re-implementing the PIN event right? Maybe the PIN event could include a message value so that we could simply reuse the PIN event rather than created another event just so we can have a different message

Copy link
Collaborator

Choose a reason for hiding this comment

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

Open to your thoughts though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This may have been an artefact of how I was wrapping the way the state machine worked and so it was seperated. But yeah more or less, that would make sense ya.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants