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

[GRIFFIN-203] "Plaintext mode" for measure creation #446

Closed
wants to merge 4 commits into from

Conversation

gavlyukovskiy
Copy link
Contributor

@gavlyukovskiy gavlyukovskiy commented Oct 25, 2018

Created json/yaml measure creation flow:
2018-10-24 21 41 44

Added ability to view measure as yaml:
2018-10-24 21 42 23

I did not add any input validation, for now only server side validation is working. I'm not quite sure if copying validation from service side is a good choice.

Created json/yaml measure creation flow
Added ability to view measure as yaml
Copy link
Member

@chemikadze chemikadze left a comment

Choose a reason for hiding this comment

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

pure awesomeness, aside some minor notes

if (response.code === '40901') {
this.toasterService.pop("error", "Error!", "Measure name already exists!");
} else {
this.toasterService.pop("error", "Error!", "Measure is not valid");
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK response typically contains valid error message that can be shown

there was similar issue in other place: https://github.com/apache/incubator-griffin/pull/423/files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

}
}

options: ITreeOptions = {
Copy link
Member

Choose a reason for hiding this comment

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

you don't have any tree component here, so this does not seem to be needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

<div class="modal fade" id="confirm" role="dialog" #modal tabindex="-1" [ngClass]="{'in': visibleAnimate}"
[ngStyle]="{'display': visible ? 'block' : 'none', 'opacity': visibleAnimate ? 1 : 0}"
(click)="onContainerClicked($event)">
<div class="modal-dialog modal-xg modal-lg">
Copy link
Member

Choose a reason for hiding this comment

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

this dialog looked not centered on screen record -- is that true or it's just me?
can you make it appear on center of the screen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just didn't captured whole window, I've just checked and it's centered.

Copy link
Member

Choose a reason for hiding this comment

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

Oh ok. Thanks for checking!

);
}

onInputChange() {
Copy link
Member

Choose a reason for hiding this comment

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

how frequently is it triggered? on every key press, or there is some throttle?
is parsing delay noticeable while typing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I can see it's done on every change, but is not noticeable on my laptop. Not sure if it will affect others, but anyway it's much easier to edit measure in normal editor and just copy-past it here :)

@chemikadze
Copy link
Member

LGTM

@guoyuepeng
Copy link
Contributor

Awesome!

@chemikadze
Copy link
Member

Looks like there are still some rendering issues:
screen shot 2018-10-25 at 2 01 33 pm

@icesmartjuan
Copy link
Contributor

icesmartjuan commented Oct 26, 2018

Awesome feature!
There seems an issue when zoom up the page, the textare is outside and the submit button can't be clicked/triggered.
It would be better if it's easy to fix the issue
image

@gavlyukovskiy
Copy link
Contributor Author

Thank you everyone for feedback, I'm not UI guy so structure was messed little bit :) I changed it to be consistent with other pages and now it looks much better.

@icesmartjuan
Copy link
Contributor

Great work, it works fine

@asfgit asfgit closed this in bcfd0e2 Oct 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants