Skip to content

Conversation

@juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Oct 10, 2019

Part of #12790

  • Remove unused stylesheets from workflowengine
  • Fix remaining comments from Workflow frontend overhaul #16706
  • Properly validate individual checks
  • Fix minor styling issues with the user agent picker
  • Allow to select multiple events
  • Handle validation on inputs before sending
  • Lots of small UI polishing
  • Async fetching for groups

@skjnldsv
Copy link
Member

Ouuh yesss!! Screenshoooots 😍

@juliusknorr juliusknorr added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 11, 2019
@juliusknorr
Copy link
Member Author

Ready for review. Best to check together with one of those

Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

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

looks good (one nitpick), still need to test

@blizzz
Copy link
Member

blizzz commented Oct 14, 2019

Not sure whether this is related to the PR, but on both personal and admin settings, saving a pdf conversion rule fails. files_accesscontrol works on the other hand.

Otherwise it looks and feels good :)

@skjnldsv
Copy link
Member

Screenshots? 🙇‍♀️

@blizzz
Copy link
Member

blizzz commented Oct 15, 2019

@skjnldsv

Overview:

Screenshot_20191015_143222

Editing:

Screenshot_20191015_143313

@jancborchardt
Copy link
Member

The typography sizes and line-heights seem to be all over the place suddenly? Is this related to the font size change? cc @ma12-co

@juliusknorr
Copy link
Member Author

Looks different to me:

image

Anyway, I rebased and pushed a fix for the saving in user context.

@juliusknorr juliusknorr force-pushed the bugfix/12790/workflow-frontend branch from d38355c to a7b0bb6 Compare October 15, 2019 18:22
@jancborchardt
Copy link
Member

jancborchardt commented Oct 15, 2019

Real nice! :) Some additional points:

  • The red is a bit bright for having white text on it. Probably better to use a black icon and text. -> Move to new operation interface for Nextcloud 18 files_accesscontrol#132
  • The "x" button in the top right deletes the flow, right? It’s a bit strange as it also floats in mid-air. Better would be a normal button left of the primary "Save" button saying "Delete".
  • It’s a bit strange that for the request time, the timezone is the present element. I’d expect the both time fields to be in the first row. The "Please enter a valid time span" can be a tooltip on whichever element is wrong, also as it can be client-side validation through input type="time".
  • And the "Europe/Berlin" dropdown doesn’t need to be so present, we can hide the border around it unless hovered/focused.
  • In the user agent dropdown: There needs to be a bit of whitespace between the icons and the text. The "Sync clients" and "Others" headers should have no background and color: var(--color-text-maxcontrast); so they have proper contrast towards the background but still subdued. And actually thinking about it, why do we have the headings "Sync clients" and "Others" anyway if "Others" is only one entry initially? I would say just make it a flat list.
  • Instead of the .icon-delete trashbin there for the list of conditions, the .icon-close x would look better.
  • The "Keep original, preserve existing PDFs is a bit wide? Could just hide the border around it and instead use the .icon-caret right of it. -> adapt to overhauled workflowengine in 18 workflow_pdf_converter#27 Also, it needs to move a slight bit to the left to be flush with the rest of the text.
  • The "Add a new filter" color is a bit too present, could be color: var(--color-text-maxcontrast); as well.

What do you think? :)

@marcoambrosini
Copy link
Member

The typography sizes and line-heights seem to be all over the place suddenly? Is this related to the font size change?

Looks much better in the last screenshot right? What was it @juliushaertl? Did you increase paddings and line heights?

@juliusknorr
Copy link
Member Author

Looks much better in the last screenshot right? What was it @juliushaertl? Did you increase paddings and line heights?

I did not change anything, this was how it looked for me even before the rebase. No idea why it looks like that for @blizzz 😕

@blizzz
Copy link
Member

blizzz commented Oct 16, 2019

Probably due to my increased default font sizes in Firefox.

@juliusknorr
Copy link
Member Author

All fixed or moved to the related prs in other apps.

It’s a bit strange that for the request time, the timezone is the present element. I’d expect the both time fields to be in the first row. The "Please enter a valid time span" can be a tooltip on whichever element is wrong, also as it can be client-side validation through input type="time".

I didn't use time as it is not supported in in IE11. Timezone is now hidden until there is a proper input provided for the time span.

@juliusknorr juliusknorr force-pushed the bugfix/12790/workflow-frontend branch from 0647805 to 33b257f Compare October 23, 2019 17:11
@blizzz
Copy link
Member

blizzz commented Oct 29, 2019

fixed another corner case in the backend, added here for simplicity

juliusknorr and others added 18 commits October 29, 2019 18:03
Signed-off-by: Julius Härtl <[email protected]>
Signed-off-by: Julius Härtl <[email protected]>
Signed-off-by: Julius Härtl <[email protected]>
Signed-off-by: Julius Härtl <[email protected]>
Signed-off-by: Julius Härtl <[email protected]>
request time, for example

Signed-off-by: Arthur Schiwon <[email protected]>
Signed-off-by: Arthur Schiwon <[email protected]>
@blizzz blizzz force-pushed the bugfix/12790/workflow-frontend branch from 24fa208 to 3f97025 Compare October 29, 2019 17:09
@blizzz
Copy link
Member

blizzz commented Oct 31, 2019

so, can we shove it in?

@gary-kim
Copy link
Member

gary-kim commented Nov 1, 2019

I'll try to review tonight.

@jancborchardt
Copy link
Member

Any more screenshots? Otherwise, if the stuff from above is fixed I think it’s good to go → we can always do a follow-up pull request. :)

@gary-kim
Copy link
Member

gary-kim commented Nov 4, 2019

I may be misunderstanding how to use this but I don't seem to be able to get a valid configuration whenever File size (upload) is being used?
Screenshot_2019-11-04 Settings - Nextcloud
It then says active if the page is reloaded.

Also, as a small nitpick, .test() is slightly faster when you don't need the result of .exec() or .match().

Copy link
Member

@gary-kim gary-kim left a comment

Choose a reason for hiding this comment

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

May be better to deal with edge cases a bit later. Everything else seems okay to me.

@blizzz
Copy link
Member

blizzz commented Nov 4, 2019

Any more screenshots? Otherwise, if the stuff from above is fixed I think it’s good to go → we can always do a follow-up pull request. :)

I undertstand it is

May be better to deal with edge cases a bit later. Everything else seems okay to me.

Thanks. Follow up issues and/or pull requests are appreciated.

@blizzz blizzz merged commit 64bfd4b into master Nov 4, 2019
@blizzz blizzz deleted the bugfix/12790/workflow-frontend branch November 4, 2019 14:01
juliusknorr added a commit that referenced this pull request Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants