Skip to content
This repository was archived by the owner on Mar 7, 2019. It is now read-only.

Conversation

@ceee
Copy link
Contributor

@ceee ceee commented Sep 26, 2017

Hi!

I've updated the UI so it better fits in the new Umbraco layout, specifically the Grid & NC editors.
Screenshots attached.

1. New UI when there is no content

archetype-1

2. List style with new buttons which only reveal on hover
(add button is always attached to the bottom, in case it is allowed to add)

archetype-2

3. New collapser style with nested Archetype

archetype-3

@kgiszewski
Copy link
Owner

@ceee, thank you for your submission. The views look great. One thing I need to ponder along with the other core team members is how this may\may not affect users who are using older versions of Umbraco core.

We presumably have a lot of folks using many different versions of Umbraco with the newest version of Archetype.

We have the advantage that Archetype is independent from the core and can pretty much do whatever we want on our own timeline.

We are similarly disadvantaged b/c Nested Content (as of v7.7) and Grid are tightly coupled to a specific core version. As the design changes with the core, that will cascade to NC\Grid.

If I knew most people using Archetype were on version x of the core, I could make some assumptions about what most of the users' experience would be. I do not have a complete picture of what that is to make an educated guess.

This could be great for current generation core users and bad for legacy users. It could be good for both.

So I'd like to see if any core team members have an opinion on this.

@leekelleher @tomfulton @kjac @mattbrailsford

@DanDiplo
Copy link

I like the new styling - consistency of UI is a good thing. It will make it easier to use both NC and Archetype depending on use case without confusing editors.

With regard to the issue of styling and version I guess we need to establish:

  1. What is lowest version of Umbraco that natively supports this styling (presuming it is using some existing styles and UI components)?
  2. How does it look in versions older than this? ie. Terrible, just different or exactly the same

@leekelleher
Copy link
Contributor

IMHO, while the styling (in this PR) does look similar to NC, I think it looks dated for Umbraco 7.7 (yup, even NC in the core looks dated to me). We tried to follow the styling of the Grid, (circa v7.2) which has since evolved from when we originally did NC.

It's pretty difficult to keep up with continuously evolving back-office design.

@kgiszewski
Copy link
Owner

Great feedback so far everyone 👍

@kjac
Copy link
Contributor

kjac commented Sep 26, 2017

@ceee great work! 👍

I have two concerns:

  1. As @kgiszewski mentions, backwards compatibility. Do the new styles rely on any Umbraco core CSS? As far as I can tell from the changed files, all the new styles are self contained so we should be good there, but a second pair of reviewing eyes wouldn't hurt.
  2. Is there room for all the icons? If every fieldset feature is turned on (enable/disable, publishing, cloning etc), there are quite a few icons. Can they all fit and do they leave any room for the fieldset labels?

I really think it's a great brush-up. We should definitively include it if at all possible. I'm not overly concerned that it might not be entirely up to snuff with the latest and greatest core UI, it's still an improvement.

@kgiszewski
Copy link
Owner

@kjac Thank you sir. The only two bits I had questions about were the two icon changes (eye and delete). not sure if they've always been around.

I will download this locally and check with the full load of features turned on for the icon spacing, etc (good thinking).

@ceee
Copy link
Contributor Author

ceee commented Sep 26, 2017

Thanks for your replies, everyone.
The CSS for sure isn't dependent on Umbraco, therefore won't change regardless of the Umbraco version.

Imho there's enough space for the icons on the right, but there's (for sure) a possibility for the label to exceed the available space, if long enough. I will test the code for long labels so it can wrap accordingly.

I do also have another potential improvement for the icons & settings management, but that should be discussed in another PR (which I will create once I am done).

@kjac
Copy link
Contributor

kjac commented Sep 27, 2017

@ceee I haven't tried your PR out, so there's a fair chance it already works this way... but here goes:

Since the fieldset icons now only show when you hover the fieldset, a solution would be to put all the icons in a wrapper DIV that's positioned absolutely in the right hand side of the fieldset header, and let the fieldset label extend below the wrapper div if necessary. That way there's plenty of room for the fieldset label - indeed more room than there is in the current implementation.

@ceee
Copy link
Contributor Author

ceee commented Sep 28, 2017

The fieldset controls are absolutely positioned, yes, but I have updated the style now so it better works with the labels. The labels have 100% now and can span across multiple lines (which wasn't possible before this PR).

1. Multi-line labels

archetype-4

2. Controls with label fading out

archetype-5

@kjac
Copy link
Contributor

kjac commented Sep 28, 2017

@ceee looks awesome 👍

@Nicholas-Westby
Copy link
Contributor

@ceee Is there a limit to the number of lines the text can span to? Wouldn't want a full article displaying in the label, for example.

@kgiszewski
Copy link
Owner

@Nicholas-Westby stole the thought right out of my mind :)

And as @kjac says, it looks good.

@ceee When you're ready for a merge, say the word so I can review locally.

@ceee
Copy link
Contributor Author

ceee commented Sep 29, 2017

@Nicholas-Westby Added a limit of 2 lines.

@kgiszewski I would say it's ready, please review :)

@kgiszewski
Copy link
Owner

Adding water to my steam-powered computer and checking this out now ... :)

@kgiszewski
Copy link
Owner

@ceee looking sharp!

image

If you see in the above image, it doesn't match your earlier screenshot. Just wanted to confirm that my image is what you intended.

@kgiszewski
Copy link
Owner

Bump... @ceee

@ceee
Copy link
Contributor Author

ceee commented Oct 2, 2017

Sorry. Weekend :D
Nope, this is not what I intended. Will push a new commit soon.

@ceee
Copy link
Contributor Author

ceee commented Oct 2, 2017

Ok, your example is limited to 50 chars because it's defined that way in the coreTinyMce (from the label service) function. I have simply increased it to 160, don't know a better way, hm.
I have only found this method which strips a label, but I guess it's the only one.

Another change:
Everytime you open a fieldset (the collapser) the controls to the right stay visible.

@Nicholas-Westby
Copy link
Contributor

@ceee Has this been tested with cross-Archetype dragging?: https://github.com/kgiszewski/ArchetypeManual/blob/master/02%20-%20Configuration.md#cross-archetype-dragging

Just want to be sure the new styles are compatible with the drop zones for the drag operations.

@ceee
Copy link
Contributor Author

ceee commented Oct 2, 2017

@Nicholas-Westby I haven't check this yet, no.
Sorry, but I am not aware of all features of Archetype (as I am not using all of them personally). Thanks for pointing this out, will make future contributions a lot easier.

I guess there shouldn't be any problems with dragging across Archetype instances, but will test it tomorrow!

@Nicholas-Westby
Copy link
Contributor

@ceee Cool. Just as a heads up, this would be the number one situation I would suggest testing to ensure it isn't failing:

empty

That is, dragging an Archetype fieldset from one property to another that is empty. When it is empty, I'm not sure there is a drop zone (at least, I don't see anything that is clearly a drop zone in the screenshot).

@kgiszewski
Copy link
Owner

@Nicholas-Westby good point. @ceee Let's confirm that and\or adjust to keep that functionality.

Sorry for the grilling, but this is all in good diligence ;)

@ceee
Copy link
Contributor Author

ceee commented Oct 3, 2017

Alright, so it did work before but I made a few changes so it looks better when cross dragging between instances.
The "Add an item" box disappears now while dragging and the drop zone is revealed (with the dash border).

1. Default view with cross-dragging enabled (and disabled too)

archetype-6

2. UI while (!) dragging

archetype-7

@kgiszewski
Copy link
Owner

@ceee great, i'm checking out this code now

@kgiszewski
Copy link
Owner

Congratulations @ceee on being the newest contributor to Archetype 👍

@kgiszewski kgiszewski merged commit c22a262 into kgiszewski:master Oct 3, 2017
@ceee
Copy link
Contributor Author

ceee commented Oct 3, 2017

Yeah, thanks a lot :-)

@kgiszewski
Copy link
Owner

The next question is usually, "so when is this getting released?". We've had a flurry of fixes go out in the last few weeks. I'm thinking we let the dust settle and shoot for maybe November 1st at the latest (unless I get impatient).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants