ui: code-editor-styling #11474
ui: code-editor-styling #11474
Conversation
shame level: HIGH lots of TODOs and also a HACK to be handled. This commit will have to be reworded as well Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
|
Hey @deblasis Firstly thanks for picking this up! I'll see if I can answer your initial questions from the ticket here:
It should stay to the same width as the other elements on the page. I'd say generally (and in the case of the screengrab you added) the width of the 'name' of the thing you are editing, so for creating a KV the width of the key input field for the KV. Actually I just checked I guess this width is the same as the grey keyline under the title, maybe this helps:
I think so, currently looks good to me. I think we may have specifically put the JSON menu outside the box to keep as much editing room as possible (I might be misremembering there) but we are moving this menu up top now anyway.:
I'm guessing we might not be able to share that, what is it you'd need here? Maybe we can figure something out.
If it was me I'd probably start from scratch, there seems to be a lot of code currently added here that looks like it adds a grey bar. I think you know this already judging by your comments but if you are copying pasting things over, you are probably bringing over a load of things you don't need.
Could you elaborate the issue with the component styling? Can I help there? Is this the copy button component or the code editor component?
I'd probably just go with something simple, either use something thats already there. Looking at the screengrab you provided in #11224 (comment), you could probably more or less just turn what you have there gray (the same gray as in the design) and give it a bit more padding/border. In the meantime I can look see if I can get you an open state if that helps. I think overall though, the most important thing here is to fix the responsive width of the editor, if we want to add another style of dropdown menus to the UI we could potentially tackle that as a separate ticket/PR. Ok so that's your initial questions covered, I'm going to go over the questions in the code in a bit and see what I can help with there, plus now we have an open PR we can use the inline code comments for specific line items. One thing I did notice from your screengrab is we've lost the little code toggle thing, but it might be hiding behind the menu, I need to look proper at your code. Be back in a bit 👍 |
|
Hi @johncowen, thanks for clarifying! I asked for the design in order to extract the CSS and make the dropdown pixel perfect but if it's not needed I am happy to make it similar enough. Looking forward to your review/guidance so that I can resume working on it. Cheers |
|
😆 Sounds good! I'm gonna be afk for a bit but I just pushed a thing I had hanging around that might help here in the meantime. I still need to look at the code properly, but this might help you with colors which I think I saw a comment about. https://consul-ui-staging-akmozj0z6-hashicorp.vercel.app/ui/docs/colors I'll come back a little later to 👀 better, I'm sure they won't end up bleeding 😆 👍 Thanks! |
not sure if it's the best practice but I used a hash to achieve conditional rendering of the help link by passing a prop to CodeEditor Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
|
Hey @deblasis , its the end of my day but I can see you adding to this so wanted to quickly drop the term 'named blocks' for Ember in for your question around passing links through into a component. They are like HTML slots. Sorry this is a little piecemeal, I've been really busy today. I wanted to leave a bit more for you to help today then I've been able to, promise I'll spend more time with you/it tomorrow. 👍 |
|
Hey @johncowen! No worries at all! I also added a tasklist in the PR description for what I think is still pending but of course, your review is gonna dictate what's left to do. |
shame level: HIGH lots of TODOs and also a HACK to be handled. This commit will have to be reworded as well Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
not sure if it's the best practice but I used a hash to achieve conditional rendering of the help link by passing a prop to CodeEditor Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
…onsul into ui/bugfix/code-editor-styling
Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
|
Just in and took a quick scan of this, and I just wanted to say real quick that this is looking awesome 👏 There will be a few things that I'd like to 'move around' I think, but let me get settled in and I'll give it a proper look 👍 |
Fantastic! Thank you @johncowen! Well, you helped, I have to give you credit. |
There was a problem hiding this comment.
This is looking super good. I left some itty bitty inline comments, but at a more general level following your lead on what you've already done here I'd like:
- Would be nice if CodeEditor had a
<:toolbar>slot/block. I can see us having different toolbars per CodeEditor/Form. If we have a <:toolbar> it means we can configure this from The Outside depending on the page we are on (the link etc would all go in the toolbar) - I think we should maybe not use
:defaultfor a name of a block, not entirely sure but from what I know about it it's potentially confusingly also the block for when no named blocks are specified, which scares me a little. Maybe use :content? I think we use that name elsewhere for the main contents of something. Bit of a nit this one all in all. - I'm still unsure as to whether we need a brand new
Toolbarcomponent, especially if we change to have a<:toolbar>slot. I need to look at this a bit further as maybe we do. I'm still curious about the fact that there seems to be a lot of CSS going on for the toolbar so I'll think about this at the same time - just wanted to share thoughts with you there - lemme know if you have anything yourself on that one.
I'm likely to come back shortly with comments on that Toolbar component, so up to you whether you want to start on changes now or wait until then (shouldn't be too long)
Lastly, again super good this, thankyou!
| display: flex; | ||
| align-items: center; | ||
| flex-direction: row; | ||
| } |
There was a problem hiding this comment.
There are a few missing rgb here. I dunno if you noticed but in the ColorSwatch docs things I linked to, if you click on the swatch it copies the value (inc. rgb) to the clipboard.
There was a problem hiding this comment.
Yes, I noticed but I was getting the wrong colours because of the dark mode I guess so I ended up writing manually (the wrong thing).
Also I noticed that there were some inconsistencies in that file already, see
andThere was a problem hiding this comment.
Yup only --tones should use the rgb syntax. Any colours without --tone should pretty much be ignored/not used.
|
|
||
| %code-editor .ember-basic-dropdown-trigger { | ||
| background-color: var(--tone-gray-900); | ||
| color: var(--black); |
There was a problem hiding this comment.
Could you use --tone-gray-000 for this please? (instead of --black)
All of the --tones are themeable --tone-gray-000 is black (in light mode) and --tone-gray-999 is white. We have this "quick dark mode" thing which just reverses the gray tones (so 000 = white and 999 = black), which gets us almost to dark mode or high contrast mode really easily.
So glad you did it like this though (but I'd like you to change) as it means I know I need to add this to our color docs 👍
There was a problem hiding this comment.
I must be using the dark mode because
comes from this:
I just pushed this specific change.
There was a problem hiding this comment.
Oh sorry wrote that wrong 🤦 its the other way around. Have a look here https://github.com/hashicorp/consul/blob/main/ui/packages/consul-ui/app/styles/base/color/ui/themes/light.scss#L1
| background-color: var(--tone-gray-900); | ||
| color: var(--black); | ||
| border-radius: 2px; | ||
| border: 1px solid; |
There was a problem hiding this comment.
Not super important but we have some more props for these if you could use those
Documentation for all this coming soon!
| height: 32px; | ||
| margin: 0 4px; | ||
| width: 0; | ||
| } |
There was a problem hiding this comment.
Feels like there a lot here, so I'm gonna take a deeper look, so up to you if you want to make changes now or wait til then. but generally:
- If you could use that CSS props for decoration things like borders etc that I linked to above that'd be great.
- Use a tone instead of black (as above)
- All in all this will need splitting into
layoutandskinfiles - but we can leave that until absolute last thing as its just cleanup/consistency - I'd rather get everything finished and then do a final clean up, but up to you if you wanted to do it now instead.
There was a problem hiding this comment.
I agree and I like the splitting, right, since I am going to refactor the Toolbar anyway now that I know how to do it ;)
I will let you know when this is ready for review again so that I don't waste too much of your time.
| @@ -24,7 +24,7 @@ $syntax-dark-gray: #535f73; | |||
| --syntax-yellow: rgb(var(--tone-yellow-500)); | |||
| } | |||
| .CodeMirror { | |||
| max-width: 1150px; | |||
| max-width: 1260px; | |||
There was a problem hiding this comment.
Oh dear 😆 it was just this 🤣 . Surprising nobody has tweaked this before now 🤔
There was a problem hiding this comment.
I know, right? It happens, it's always a single line of code that is holding things together or that breaks everything :)
| {{yield}} | ||
| </div> | ||
| </nav> | ||
| </nav> |
There was a problem hiding this comment.
afaik nav should only be used for site / app main nav (or sub nav or whatever) but its generally a way to navigate around the site / app rather than for a toolbar thing. I'd just stick with div here unless you can suggest something else? (I'd be concerned about suggesting menu as I vaguely remember browser difference/complications from a while back but I've no idea if thats still relevant, so unless you know more I'd maybe err on the side of caution with a div)
There was a problem hiding this comment.
I stole this from Vault and I played ignorantly to be honest but yeah, it is especially weird because we have nested nav too. I'll change that.
Sorry missed this. Ember doesn't auto include the CSS files for the component (we have work hanging around to make it do this, which funnily enough we were chatting about amongst the team today). We currently manually include component CSS files here: https://github.com/hashicorp/consul/blob/main/ui/packages/consul-ui/app/styles/components.scss (this ^ file is then included by the app.css file which Ember pulls in) I'd probably wait until I've stared at the toolbar stuff a little more though before moving stuff around as I'm just thinking whether we want to include the styling for the toolbar in the CodeEditor component ( I can't think of a reason why we'd use a this toolbar component as part of anything other than this CodeEditor right now 🤔 )
👀 looking now I did notice a little bug with our CopyButton tho! It doesn't do anything if the thing you are trying to copy is empty (we can add that as a separate task afterwards, just an FYI that this is our problem I think) |
Well, you made me RTFM a little by pointing me to the right "chapter" 😊
Sounds sensible, If you are expecting various use cases then it definitely makes sense. I can definitely refactor this... my train of thought about this continues in point 3 below.
I used the default named block exactly because it's equivalent to putting HTML/Components inside the receiving Component and also Ember doesn't allow you to mix named blocks with the unnamed one. <CodeEditor
@syntax="hcl"
@readonly={{true}}
>
<:title>
Rules <a href="{{env 'CONSUL_DOCS_URL'}}/guides/acl.html#rule-specification" rel="help noopener noreferrer" target="_blank">(HCL Format)</a>
</:title>
<:default>
<Consul::NodeIdentity::Template
@name={{item.Name}}
/>
</:default>
</CodeEditor>It has a <CodeEditor
@syntax="hcl"
@readonly={{true}}
>
<Consul::NodeIdentity::Template
@name={{item.Name}}
/>
</CodeEditor>which renders that block inside the Anyway... I guess maybe we can simply rename
If the toolbar is going to be used in multiple places, it should have its own CSS and be configurable without exposing too much implementation details IMHO. I wanted a separate component because I was "thinking forward" but the fact that I wasn't able to add the SCSS stopped me... now you gave me the power by pointing out how to do that 😂
I'll fix the blatant mistakes that you identified commenting atomically on each one if needed and then I'll move forward with the refactoring.
No worries! You are welcome! |
Bit weird I know, but if you put a K, half the reason why this is a bit of a difficult one for me to suggest which way to go is because of ongoing work over here: https://github.com/hashicorp/consul/tree/main/ui/packages/consul-ui/app/components/text-input That was building up to a replacement for this CodeEditor component completely. Basically I'm conscious of you doing stuff that we may only use temporarily before moving some of your stuff into that newer component completely (that might never happen). I guess all in all if we ignore that work is there and you are fine us potentially re-purposing your stuff in that new component at some point in the distant future (the new component will need the ability to show a configurable toolbar) then I think we should:
Questions from me:
Hope this is ok, I'm pretty sure this is mostly just moving what you've done around a bit - if its a bit over the top then lemme know and we can probably do less. Oh I keep forgetting, I keep meaning to ask you to do a housekeeping task or 2 also. I'm not sure if you can turn this into a Draft PR or not, if you can that means we can lose the WIP out the title. Also we usually prefix all UI PRs (and therefore commits) with Just got your comment in so that ^ is before reading it, this is after reading it (GH tennis!) re: All in all I think we should just rename
I think the important thing here is it will be used in multiple places but only ever when using the CodeEditor (for the immediate future at least). Maybe think of Anyway, theres quite a lot here ^ now 😬 , so if you are like "Wo wo wo hold on a second" I'd also be happy to get this in without to much more moving around. I think visually its looks great as is, definitely no eye bleeding 😆 Oh also, if you fancy doing a little more once we are done here, there're docs to fill out in the code-editor/README.mdx but lets finish up here and I can explain how that works if you fancy doing that also (np if not) |
Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
|
Good stuff, I am gonna ping you as soon as there's stuff to review and/or questions. Let me know if I should reword the commit with the Cheers |
- :label named block - :tools named block - :content named block - code and CSS cleanup - CodeEditor.mdx Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
|
Hi @johncowen!
I am totally fine with that, the way I see it: code is per-se an everchanging thing that doesn't have a name on it. I have learnt a tiny bit of ember, worked with nice people, I am good already 😊 one day I'll tell my kids I contributed to consul and they will be proud of me for that too 😉
I think I addressed all these points now, I just named :toolbar to :tools instead because it sounded better since in that block we only put tools and the toolbar is at the parent level.
It was pre-cleanup code that I carelessly stole from Vault 🙈 if I had the designs I would have designed everything from scratch but you know... developers, lazy people.
No worries! This is like a walk in the park to be honest
LOL, agreed, Slack would have been a better way to communicate but despite that, we managed 😄
Done, I mean, winging it, let me know if you like what you see. |
|
Sorry for the radio silence here, just a quick FYI in the meantime just waiting to go through this with the team, and then there might be the odd tweak before we merge. |
|
Thanks for the update and no worries @johncowen, take your time. |
There was a problem hiding this comment.
There are a few little things that are tinsy details that are more stylistic than anything so I'm good to merge this as is. The only thing is if we could make the docs page render properly (I added a suggestion which should make it render properly) then we are good to go. You should be able to see this page (if you haven't already) by clicking the [Eng Docs] link in the top right of the UI when you are running in dev mode.
Lemme know once that's done and we can get it merged.
Thanks for your help here 🎉
Co-authored-by: John Cowen <johncowen@users.noreply.github.com>
|
I see, sounds good! Suggestion just committed. You are very welcome 😄 |
|
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/501457. |
This PR should address #11224, submitting it as WIP so that we can agree on the right way forward.
It also lacks functionality.
The missing deliverables are marked as
TODOsand there's also a blatantHACK(first time using Ember so forgive me if I made blatant mistakes).There's also commented code with an explanation of what I was trying to achieve.
Eventually, I'll figure it out 😊
Tasklist:
copy-buttononClickevent handler, works fine for KV 😕layout.scssandskin.scsslabelnamed blocktoolbartoolsnamed blockcontentnamed block