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

1.0.0-beta #1

Closed
wants to merge 1 commit into from
Closed

1.0.0-beta #1

wants to merge 1 commit into from

Conversation

DustinBrett
Copy link
Owner

No description provided.

@DustinBrett DustinBrett self-assigned this Oct 8, 2020
@@ -0,0 +1,362 @@
{
Copy link
Owner Author

Choose a reason for hiding this comment

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

@@ -0,0 +1,4 @@
{
Copy link
Owner Author

Choose a reason for hiding this comment

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

@@ -0,0 +1,9 @@
root = true
Copy link
Owner Author

Choose a reason for hiding this comment

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

"import/prefer-default-export": "off",
"jsx-a11y/click-events-have-key-events": "off",
"lines-between-class-members": "off",
"react-hooks/exhaustive-deps": "off",
Copy link
Owner Author

Choose a reason for hiding this comment

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

Make this warn until all edge cases where it's needed can be tested.

"global-require": "off",
"import/no-unresolved": "off",
"import/prefer-default-export": "off",
"jsx-a11y/click-events-have-key-events": "off",
Copy link
Owner Author

Choose a reason for hiding this comment

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

Make this warn and eventually deal with these issues.

@@ -0,0 +1,71 @@
# For most projects, this workflow file will not need changing; you simply need
Copy link
Owner Author

Choose a reason for hiding this comment

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

Added automatically via GitHub and formatted by Prettier.

@@ -0,0 +1,19 @@
{
Copy link
Owner Author

Choose a reason for hiding this comment

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

{
"extends": "stylelint-config-recommended",
"rules": {
"at-rule-no-unknown": [
Copy link
Owner Author

Choose a reason for hiding this comment

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

Could https://www.npmjs.com/package/stylelint-scss negate the need for this rule?

@DustinBrett DustinBrett marked this pull request as ready for review October 8, 2020 05:12
@@ -0,0 +1,77 @@
import styles from '@/styles/Programs/Dos.module.scss';
Copy link
Owner Author

Choose a reason for hiding this comment

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

Logic of import order (space between "types"):

  • Files (png, svg, json, etc.)
  • Styles
  • Types modules then exports (alphabetical)
  • Modules then exports (alphabetical)

NOTE: Perhaps add a style guide?


import type { DosMainFn, DosRuntime } from 'js-dos';
import type { DosCommandInterface } from 'js-dos/dist/typescript/js-dos-ci';
import type { AppComponent } from '@/types/utils/programs';
Copy link
Owner Author

@DustinBrett DustinBrett Oct 8, 2020

Choose a reason for hiding this comment

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

Alphabetize AppComponent

import type { WindowWithDosModule } from '@/types/components/Programs/dos';

import { useEffect, useRef } from 'react';
import { getLockedAspectRatioDimensions } from '@/utils/dos';
Copy link
Owner Author

@DustinBrett DustinBrett Oct 8, 2020

Choose a reason for hiding this comment

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

Alphabetize getLockedAspectRatioDimensions & focusClosestFocusableElementFromRef.

import { TITLEBAR_HEIGHT } from '@/utils/constants';
import { focusClosestFocusableElementFromRef } from '@/utils/elements';

const dosOptions = {
Copy link
Owner Author

Choose a reason for hiding this comment

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

Rename jsDosOptions to not confuse with DOS in general.

onprogress: () => {} /* eslint @typescript-eslint/no-empty-function: off */
};

export const loaderOptions = {
Copy link
Owner Author

Choose a reason for hiding this comment

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

Maybe an exported app should be an object of options and the component.

bgColor: '#000000'
};

const Dos: React.FC<AppComponent> = ({
Copy link
Owner Author

Choose a reason for hiding this comment

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

AppComponent needs to be rethought. Perhaps ProgramProps. Then make it an & of several clearly named types which represent the sources for Window/App passed params.


const Dos: React.FC<AppComponent> = ({
args = ['-c', 'CLS'],
file: { url, name = '' } = {},
Copy link
Owner Author

@DustinBrett DustinBrett Oct 8, 2020

Choose a reason for hiding this comment

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

Remove the need for = {}, make file have a default as well as its key/values.

}) => {
let ci: DosCommandInterface;
const canvasRef = useRef<HTMLCanvasElement>(null);
const loadMain = (main: DosMainFn, prependedArgs: string[] = []) =>
Copy link
Owner Author

Choose a reason for hiding this comment

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

Move loadMain and loadDos outside of the component to avoid recreation on re-renders.

let ci: DosCommandInterface;
const canvasRef = useRef<HTMLCanvasElement>(null);
const loadMain = (main: DosMainFn, prependedArgs: string[] = []) =>
main([...prependedArgs, ...args]).then((value) => {
Copy link
Owner Author

Choose a reason for hiding this comment

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

Change value to dosCI.

: {};

useEffect(() => {
const { current: canvasElement } = canvasRef as {
Copy link
Owner Author

Choose a reason for hiding this comment

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

Create a type for this canvasRef

const { current: canvasElement } = canvasRef as {
current: HTMLCanvasElement;
};
const { Dos: DosModule } = window as WindowWithDosModule;
Copy link
Owner Author

@DustinBrett DustinBrett Oct 8, 2020

Choose a reason for hiding this comment

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

Rename to JsDosModule and WindowWithJsDosModule

}
};
const maximizedStyle = maximized
? getLockedAspectRatioDimensions(loaderOptions.width, loaderOptions.height)
Copy link
Owner Author

Choose a reason for hiding this comment

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

getLockedAspectRatioDimensions, is this for maximum of window? Rename possibly.

return (
<div className={styles.dos} style={maximizedStyle}>
<canvas
onTouchStart={focusClosestFocusableElementFromRef(canvasRef)}
Copy link
Owner Author

Choose a reason for hiding this comment

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

Element or something more specific like Window?

return () => {
ci?.exit();
};
}, []);
Copy link
Owner Author

@DustinBrett DustinBrett Oct 8, 2020

Choose a reason for hiding this comment

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

Is canvasRef a dep?

DosModule(canvasElement, dosOptions).then(loadDos);

return () => {
ci?.exit();
Copy link
Owner Author

Choose a reason for hiding this comment

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

Can this be moved out of {}?


import type { AppComponent } from '@/types/utils/programs';

import { basename } from 'path';
Copy link
Owner Author

Choose a reason for hiding this comment

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

Reorganize basename to L8 and ProcessContext to L10

import { ProcessContext } from '@/contexts/ProcessManager';
import { ROOT_DIRECTORY } from '@/utils/constants';

export const loaderOptions = {
Copy link
Owner Author

Choose a reason for hiding this comment

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

If keeping loaderOptions then move to export area

@DustinBrett DustinBrett deleted the beta branch March 7, 2021 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant