diproche / webinterface

1 stars 0 forks source link

Ronja kaehne/example page #84

Closed RonjaKaehne closed 5 years ago

Korosensei42 commented 5 years ago

In general I really like the idea of giving an explanation only when hovering over it.

However, it is not very intuitive that this option even exists. The user may only encounter this functionality if he either marks the whole text or hovers over the explanation by himself, which seems random when you don't know about this functionality.

I would suggest to change this to something similar to the way Discord handles it. See for example here: https://gyazo.com/a078f1c07440bf41d335287983266c3b

The only thing I would change about this Discord-way is that we should have the ability to hide text again, just like you are doing right now. So I would only suggest some highlight of the region that can be uncovered by hovering over it.

Other than this, this looks fine. So I would approve this PR if you comment on my remark

RonjaKaehne commented 5 years ago

Hey, i added a new structure that allows us to pick and show predefined examples by pressing a button. we have to add some other example and should store them into a seprated file (see #73 ) so we could acces them by there. check out the functionality of the example generator :) actually the code pick a random example from all existing examples in the list. we also can increment the list by each cick on the button for sure. this one was just a possible suggestion

Korosensei42 commented 5 years ago

I'm gonna get to #73 tomorrow, most likely

TimothyGillespie commented 5 years ago

Issue #98 and #99 will appear with the pull request.

TimothyGillespie commented 5 years ago

@Korosensei42 Please have a quick look over this and merge it if you seem fit. You could get started on #99 if you wish to do so then.

Korosensei42 commented 5 years ago

@TimothyGillespie I thought I might as well do #99 now and merge afterwards. This way we don't need an additional PR.

Regarding merging this: I get 3 errors in Diproche/github/webinterface/src/components/propositionalLogic/examplePage.tsx., so merging would not feel appropriate right now. Do you have a suggestion on how to fix them?

TimothyGillespie commented 5 years ago

What errors are you getting? I don't see any and the CI says it's passing too?

Korosensei42 commented 5 years ago

I get three errors:

  1. In line 5 with exampleProofList. Currently it looks like this:

    const exampleProofList: Array<Array<[string, string]>> = data.examples;

    Error description:

    Type '((string | string[])[] | (string | string[])[][])[][]' is not assignable to type '[string, string][][]'.
    Type '((string | string[])[] | (string | string[])[][])[]' is not assignable to type '[string, string][]'.
    Type '(string | string[])[] | (string | string[])[][]' is not assignable to type '[string, string]'.
      Type '(string | string[])[]' is missing the following properties from type '[string, string]': 0, 1ts(2322)
  2. In line 28 with exampleNumber, false (arguments of renderProof). Currently it looks like this:

    public render() {
        return <div className={styles.site}>
            {this.renderExampleButtons()}
            <table className={styles.example} id="solve">
                {this.renderProof(exampleNumber, false)}
            </table>
        </div >;
    }

Error description:

Expected 0 arguments, but got 2.ts(2554)
  1. In line 61 with proofPart.

Currently it looks like this:

            renderedPart.push(<tr>
                <td>{line[0]}</td>
                <td className={scssClass} onClick={() => this.showExplanation(index, proofPart)}>{explanation}</td>
            </tr>);

Error Description: Expected 1 arguments, but got 2.ts(2554)

Now, fixing 2 and 3 would be no big deal, I guess. However, I don't know what would be correct for 1.

I also pulled, so I don't get why only I get these errors. Btw: Sorry for the wall of text, but I figured I'd better be as detailed as possible.

TimothyGillespie commented 5 years ago

The errors should be fixed now. I don't know why I didn't get them myself first. I removed the lines with Aufgabe: ... since the initial idea would be to have this be 1:1 typable into the proofEditor and work and also the first line should state what we are trying to prove. These are not supposed to be task yet but simply full examples. The tasks will be on another page. If you think they need an header we can still add one. @Korosensei42 did you intend to use your addition into the json as predefined tasks?

Korosensei42 commented 5 years ago

@TimothyGillespie It doesn't really matter if we include them here, since, as you said, we can still create a headline if we want to use the examples as tasks. So deleting them here is totally fine with me,