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

implemented an autocomplete feature for the "move page" (version 2) #867

Merged
merged 1 commit into from Feb 12, 2016

Conversation

Projects
None yet
2 participants
@gmeiser

gmeiser commented Feb 1, 2016

Hi folks,

new year, new try.
Implemented the autocomplete feature for moving tests again.
This time hopefully the way Arjan recommended it.

Please have a look at it. Feedback appreciated.

Greetings Gordon

@amolenaar amolenaar merged commit 2c1944e into unclebob:master Feb 12, 2016

amolenaar added a commit that referenced this pull request Feb 12, 2016

Merge pull request #867 from gmeiser/master
implemented an autocomplete feature for the "move page" (version 2)
@amolenaar

This comment has been minimized.

Show comment
Hide comment
@amolenaar

amolenaar Feb 12, 2016

Collaborator

The PR looks nice. Only when I tried it the datalist remained empty. I can't figure out how it should work.

Collaborator

amolenaar commented Feb 12, 2016

The PR looks nice. Only when I tried it the datalist remained empty. I can't figure out how it should work.

@amolenaar

This comment has been minimized.

Show comment
Hide comment
@amolenaar

amolenaar Feb 12, 2016

Collaborator

I did some tweaks in 1723f91, you might want to review them.

Collaborator

amolenaar commented Feb 12, 2016

I did some tweaks in 1723f91, you might want to review them.

amolenaar added a commit that referenced this pull request Feb 12, 2016

Merge pull request #867 from gmeiser/master
implemented an autocomplete feature for the "move page" (version 2)

amolenaar added a commit that referenced this pull request Feb 12, 2016

@gmeiser

This comment has been minimized.

Show comment
Hide comment
@gmeiser

gmeiser Feb 15, 2016

Hey Arjan,
I just took a look at your tweaks.

Seems to me like you are using a lot of already existing methods and functionality which I didn't know (or didn't find :) ) and the result is pretty much the same. However the preceding "dot" and the fact that the datalist is also showing all testcases are differing from my suggestion.
I guess the "dot" is neither a problem for me nor the rest of the users but I would suggest adapting one line in your code.

In RefactorPageResponder I would change line 68 from:

if (!thisPagePath.equals(pagePath) && !pagePath.isEmpty()) {

to

if (!thisPagePath.equals(pagePath) && !pagePath.isEmpty() && page.getData().hasAttribute("Suite")) {

This way the datalist is only showing the names of suites and excluding tests. This way the datalist remains clearly arranged.
That do you think about it?

Greetings Gord

gmeiser commented Feb 15, 2016

Hey Arjan,
I just took a look at your tweaks.

Seems to me like you are using a lot of already existing methods and functionality which I didn't know (or didn't find :) ) and the result is pretty much the same. However the preceding "dot" and the fact that the datalist is also showing all testcases are differing from my suggestion.
I guess the "dot" is neither a problem for me nor the rest of the users but I would suggest adapting one line in your code.

In RefactorPageResponder I would change line 68 from:

if (!thisPagePath.equals(pagePath) && !pagePath.isEmpty()) {

to

if (!thisPagePath.equals(pagePath) && !pagePath.isEmpty() && page.getData().hasAttribute("Suite")) {

This way the datalist is only showing the names of suites and excluding tests. This way the datalist remains clearly arranged.
That do you think about it?

Greetings Gord

@amolenaar

This comment has been minimized.

Show comment
Hide comment
@amolenaar

amolenaar Feb 15, 2016

Collaborator

Thanks for your feedback. I appreciate it!

Why should the move only be limited to Suite pages? It should be possible to move ordinary pages just as easy, I think. I've seen setups where test pages have subpages (e.g. a particular SetUp page, or additional tests that you do not want in the main test page for readability).

I found one flaw, though: the subpages of the to be moved page are also shown in the list. I should filter those out.

Collaborator

amolenaar commented Feb 15, 2016

Thanks for your feedback. I appreciate it!

Why should the move only be limited to Suite pages? It should be possible to move ordinary pages just as easy, I think. I've seen setups where test pages have subpages (e.g. a particular SetUp page, or additional tests that you do not want in the main test page for readability).

I found one flaw, though: the subpages of the to be moved page are also shown in the list. I should filter those out.

@amolenaar amolenaar added this to the Next release milestone Feb 15, 2016

@gmeiser

This comment has been minimized.

Show comment
Hide comment
@gmeiser

gmeiser Feb 15, 2016

Hmm, okay. Only because I am always using a suite as a "folder" for tests it doesn't mean that all people are doing it the same way. So your solution is much more universal.

For me (and maybe others) it would help a lot if I can detect suites with one quick glance.
One possibility would be the following:

if (!thisPagePath.equals(pagePath) && !pagePath.isEmpty()) {
  if (page.getData().hasAttribute("Suite")) {
    pageNames.add(pagePath.toString()+"\" label=\"suite");
  }
  else pageNames.add(pagePath.toString());
}

This will print "suite" on the right side of the dropdown in a column with a suite name (static pages could be handled the same way).
Indeed this is only working on my Chrome. Firefox will NOT show the hint and on IE 11 the whole datalist popup looks like crap, mixing suite and name columns like crazy.

As HTML5 is not implemented the same way in all browsers I guess it would be hard to fullfill my wish in a decent way. :(

gmeiser commented Feb 15, 2016

Hmm, okay. Only because I am always using a suite as a "folder" for tests it doesn't mean that all people are doing it the same way. So your solution is much more universal.

For me (and maybe others) it would help a lot if I can detect suites with one quick glance.
One possibility would be the following:

if (!thisPagePath.equals(pagePath) && !pagePath.isEmpty()) {
  if (page.getData().hasAttribute("Suite")) {
    pageNames.add(pagePath.toString()+"\" label=\"suite");
  }
  else pageNames.add(pagePath.toString());
}

This will print "suite" on the right side of the dropdown in a column with a suite name (static pages could be handled the same way).
Indeed this is only working on my Chrome. Firefox will NOT show the hint and on IE 11 the whole datalist popup looks like crap, mixing suite and name columns like crazy.

As HTML5 is not implemented the same way in all browsers I guess it would be hard to fullfill my wish in a decent way. :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment