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

Elektra Web 1.5 #2029

Closed
wants to merge 378 commits into from
Closed

Conversation

omnidan
Copy link
Contributor

@omnidan omnidan commented May 26, 2018

@markus2330 please review


Release notes

Many UX improvements from the usability test!

  • search completely reworked - it does not act as a filter on already opened keys anymore, and instead searches the whole key database - feedback from the search was also greatly improved (pulsating while searching, glowing blue when done)
  • added "abort" buttons to dialogs to revert actions
  • added "create array" button to easily create arrays
  • removed confirmation dialog before deletion (undo can be used instead)
  • created a docker image: elektra/web
  • small fixes:
    • updated visibility levels
    • removed "done" button in main view
    • fixed issues with the opener click area
    • remove metakeys when they are set to the default value or empty/0

@omnidan omnidan mentioned this pull request May 26, 2018
6 tasks
@omnidan omnidan changed the title WIP: Elektra Web 1.5 Elektra Web 1.5 May 26, 2018
Copy link
Contributor

@markus2330 markus2330 left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! Some small comments.

@@ -11324,14 +11324,6 @@
}
}
},
"react-tap-event-plugin": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please write docu how to udpate this file?

@@ -99,7 +99,12 @@ export default class SettingsDialog extends Component {
}
// persist value to kdb and show notification
const { timeout } = this.state[key] || {}
setMeta(key, value)

// remove metakey when setting to 0
Copy link
Contributor

Choose a reason for hiding this comment

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

and empty value?

Copy link
Contributor

@markus2330 markus2330 left a comment

Choose a reason for hiding this comment

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

Thank you for improving the docu!

'binary',
+ 'check/something',
]
```
Copy link
Contributor

Choose a reason for hiding this comment

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

And then, how to do validation or similar?

@omnidan
Copy link
Contributor Author

omnidan commented May 31, 2018

@markus2330 all tasks from #2032 should be done now, I also tested with Firefox ESR.

you can review already, because only the docker container for the live demo is missing now.

@markus2330
Copy link
Contributor

markus2330 commented May 31, 2018

Thank you!

The docker container would make it easier to test (and reproduce) 😉

@omnidan
Copy link
Contributor Author

omnidan commented May 31, 2018

@markus2330 I created a Dockerfile (see #1901 (comment)). I also released an image, but it is still 1.4.0 (as the Dockerfile builds from the official git repo). I am also building a beta release docker image for 1.5.0 right now.

@markus2330
Copy link
Contributor

I am okay with merging this PR so that creating the Docker image is easier. If everything is finished, please set "ready to merge".

@@ -0,0 +1,34 @@
FROM ubuntu:16.04
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why this file is not in scripts/docker/?

@omnidan
Copy link
Contributor Author

omnidan commented May 31, 2018

@markus2330 I already created a 1.5.0-beta image, no worries. you can install it via:

docker run -d -it -p 33333:33333 -p 33334:33334 elektra/web:1.5.0-beta

@ingwinlu
Copy link
Contributor

why the two ports? you are missing EXPOSE entries in the Dockerfile. You might also be interested in removing some layers to reduce image size and speed up builds.

Additionally you don't have a PID1 that controls signals sent to the container.

@markus2330
Copy link
Contributor

@omnidan Thank you, works for me. As said, mountpoints would make the image more interesting. (Maybe mount /tmp/hosts to not destroy anything.) What about providing a start-web script instead of creating it only for the docker image? And release notes are missing! You are welcome to highlight your changes.

@ingwinlu Thank you for reviewing the Dockerfile!

@markus2330
Copy link
Contributor

@omnidan small things: "edit value" does not automatically focus to the edit field. Please also check if tabs work as wanted (they also cycle in the background of the dialog). And numbering of arrays sometimes does not work (if I clicked somewhere else before.) Should I create an issue for this?

@omnidan
Copy link
Contributor Author

omnidan commented Jun 1, 2018

@ingwinlu thanks for the hints! I added the two ports via EXPOSE. One port is for elektrad (33333), the other for webd/client (33334). I also tried reducing the image size by following instructions from: https://blog.florianlopes.io/5-tips-to-reduce-docker-image-size/

I'm not sure what you mean by "you don't have a PID 1" - I found out that nodejs is not supposed to run as PID 1 in docker: https://www.elastic.io/nodejs-as-pid-1-under-docker-images/

@omnidan
Copy link
Contributor Author

omnidan commented Jun 1, 2018

@markus2330

"edit value" does not automatically focus to the edit field.

thanks for the hint - I fixed this!

Please also check if tabs work as wanted (they also cycle in the background of the dialog).

unfortunately trapping the focus in the dialog does not work with the current version of the UI library I use (MUI): mui/material-ui#4384

I tried using an external library to achieve this, but with the external library I can only trap focus within the dialog contents, which excludes the dialog buttons. so when I enable the focus trap, it is not possible to navigate to the dialog buttons anymore.

focus trapping dialogs are implemented in the latest beta version of MUI, however: mui/material-ui#10458 - should I make an issue about things that can be fixed by upgrading to MUI 1.0 when it comes out (selecting text when pressing the copy button, trapping focus in dialogs)?

And numbering of arrays sometimes does not work (if I clicked somewhere else before.)

Where did you click? I'm not sure how to reproduce this.

@omnidan
Copy link
Contributor Author

omnidan commented Jun 1, 2018

@markus2330

Thank you, works for me. As said, mountpoints would make the image more interesting. (Maybe mount /tmp/hosts to not destroy anything.)

We could push an example kdb export to a git repo and then pull that to build the docker image for the live demo (I would create a separate image for that though).

And release notes are missing! You are welcome to highlight your changes.

I added them to the initial post of this PR!

@markus2330 markus2330 mentioned this pull request Jun 2, 2018
24 tasks
@markus2330
Copy link
Contributor

markus2330 commented Jun 2, 2018

Thank you, looks great.

But please describe changes in changelog (news). (your text in this PR is quite good, but it should be in doc/news/_preparation_next_release.md)

You can also finish (some) tasks of #2037 here.

@markus2330
Copy link
Contributor

jenkins build all please

@markus2330
Copy link
Contributor

Please add some info to the release notes.

@markus2330
Copy link
Contributor

Thank you. Seems like you need to rebase.

@omnidan
Copy link
Contributor Author

omnidan commented Jun 18, 2018

@markus2330 done!

@markus2330
Copy link
Contributor

Thank you! Could you please rebase to get rid of the old commits.

@ingwinlu Can you take a look at the docker file?

@omnidan
Copy link
Contributor Author

omnidan commented Jun 18, 2018

@markus2330 I already rebased, what do you mean "get rid of the old commits"?

@markus2330
Copy link
Contributor

The github view shows 250+ commits. Something seems to be wrong here.

@markus2330
Copy link
Contributor

And maybe it is a good idea to split the docker files and the WebUI changes.

@omnidan
Copy link
Contributor Author

omnidan commented Jun 18, 2018

The github view shows 250+ commits. Something seems to be wrong here.

@markus2330 that is because I rebased and it added all commits from upstream master to the branch, they should not be duplicated after merging (it looked the same way when I rebased my previous PR).

BTW: you can see that the PR only has my changes when going to the "files" tab: https://github.com/ElektraInitiative/libelektra/pull/2029/files

@omnidan
Copy link
Contributor Author

omnidan commented Jun 18, 2018

And maybe it is a good idea to split the docker files and the WebUI changes.

I will remove them from this PR for now and create a new PR with the updated docker files tomorrow.

@markus2330
Copy link
Contributor

There is still something very wrong in this PR. There are 250+ commits.

@omnidan
Copy link
Contributor Author

omnidan commented Jun 19, 2018

@markus2330 as I said, that is a result of rebasing, it happened in the previous PR too. Look at the "Files changed" tab, it does not actually contain the changes of the commits.

@omnidan omnidan mentioned this pull request Jun 19, 2018
@omnidan
Copy link
Contributor Author

omnidan commented Jun 19, 2018

see #2099

@omnidan omnidan closed this Jun 19, 2018
@ingwinlu
Copy link
Contributor

@omnidan out of curiosity: how do you rebase?

@markus2330
Copy link
Contributor

it happened in the previous PR too.

This does not say it was correct. It also messed up the history back then.

The problem is that the common commit is old (26.05) and that you rebased something that already contained a merge commit.

A rebase should be very simple in your case, nobody else modified the directories you are working in.

You can also create a new PR if it is easier for you.

@omnidan
Copy link
Contributor Author

omnidan commented Jun 19, 2018

@ingwinlu https://help.github.com/articles/syncing-a-fork/ and then git rebase master

@markus2330
Copy link
Contributor

I do not think you should mix these two.

You can either merge with -ff or even -ff-only or do a pull with --rebase.

@omnidan omnidan mentioned this pull request Jun 19, 2018
@ingwinlu
Copy link
Contributor

Make sure upstream is really pointing at the correct repository via git remote -v.
Your process seems fine to me otherwise.

@omnidan
Copy link
Contributor Author

omnidan commented Jun 19, 2018

@ingwinlu I read that it might be the way I created the branch, not the way I rebased: https://stackoverflow.com/a/40413455

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.

7 participants