Skip to content

feat(extensions): add helper header above extensions search #2501

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jan 14, 2021
Merged

feat(extensions): add helper header above extensions search #2501

merged 2 commits into from
Jan 14, 2021

Conversation

jsjoeio
Copy link
Contributor

@jsjoeio jsjoeio commented Dec 21, 2020 •
edited
Loading

Add a short message above the search box on the Extensions panel. This helps explain the extension divergence to the user.

If they click dismiss, it stores an item in localStorage to prevent the message from showing up on subsequent loads.

Builds off the work previously done by @cmoog in #2185

Screenshots

2020-12-21 10 12 37

And it uses the correct colors based on theme:

IMG_0042.MP4

Checklist

@jsjoeio jsjoeio self-assigned this Dec 21, 2020
@jsjoeio jsjoeio changed the title feat (extensions): add helper header above extensions search feat(extensions): add helper header above extensions search Dec 21, 2020
@jsjoeio jsjoeio requested review from code-asher and nhooyr December 23, 2020 17:00
code-asher
code-asher approved these changes Jan 5, 2021
View reviewed changes
Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

Looks great!

@nhooyr
Copy link
Contributor

nhooyr commented Jan 5, 2021

Love the gif btw 🔥

@jsjoeio jsjoeio force-pushed the issue-2132-help-info-extension-search-view branch 2 times, most recently from dc2445c to 7060f60 Compare January 7, 2021 18:25
@jsjoeio jsjoeio requested a review from nhooyr January 7, 2021 18:28
@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jan 7, 2021

rsync: change_dir "/github/workspace//typings/pluginapi.d.tsrelease" failed: No such file or directory (2)
2057
rsync error: some files/attrs were not transferred (see previous errors) (code 23) at main.c(1207) [sender=3.1.3]

I’m not totally sure why the release step is failing. Any ideas?

@code-asher
Copy link
Member

code-asher commented Jan 8, 2021 •
edited
Loading

Ahh, sorry about the failed release step, it's my fault. I pushed a fix to master, please feel free to rebase.

@jsjoeio jsjoeio force-pushed the issue-2132-help-info-extension-search-view branch from 044d992 to 93ca456 Compare January 8, 2021 17:51
@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jan 11, 2021

Bump @nhooyr - ready for a second review

@nhooyr
Copy link
Contributor

nhooyr commented Jan 11, 2021

Can confirm things work for me!

But the links appear to be styled very hard to read for some reason?

Are we maybe missing a class on the links or some styles?

Screen Shot 2021-01-11 at 12 43 38 PM

@nhooyr
Copy link
Contributor

nhooyr commented Jan 11, 2021

Like there's little contrast compared to the other links like:
Screen Shot 2021-01-11 at 12 44 42 PM

@nhooyr
Copy link
Contributor

nhooyr commented Jan 11, 2021

@jsjoeio make sure to squash the really long Update file commits generated by github.

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jan 11, 2021

Can confirm things work for me!

But the links appear to be styled very hard to read for some reason?

Are we maybe missing a class on the links or some styles?

Hmm! Good catch. Let me look into it.

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jan 11, 2021

@jsjoeio make sure to squash the really long Update file commits generated by github.

You got it!

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jan 11, 2021

Like there's little contrast compared to the other links like:

I’m having trouble figuring this out on iPad since I don’t have access to DevTools to inspect the styles 🤔 I’m looking at the Welcome page and don’t see anything jumping out at me. The links in the list have no classes so i’m not sure where the link color is coming from.

4FDD2F99-E546-4194-8536-85CDC6745F09

The easiest solution would be to hard-code the link style to match the rest of code-server but I worry that wouldn’t work well across various themes. I’ll keep investigating.

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jan 11, 2021 •
edited
Loading

Well...hard-coding the color works. Not sure what else to try. Any thoughts/suggestions @nhooyr ?

8B5C8CEE-B40C-446B-A4CD-271AE6B82FF5

@jsjoeio jsjoeio force-pushed the issue-2132-help-info-extension-search-view branch from 93ca456 to 3af5c25 Compare January 11, 2021 23:46
nhooyr
nhooyr reviewed Jan 12, 2021
View reviewed changes
Copy link
Contributor

@nhooyr nhooyr left a comment

Choose a reason for hiding this comment

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

Happy to merge as is but one downside of hard coding the link color like we have is that the :visited style doesn't take any affect and so the links don't turn purple anymore which sucks.

I understand @code-asher has some ideas to fix this?

@nhooyr
Copy link
Contributor

nhooyr commented Jan 12, 2021

Or one more I should say, like you said themes will also be a problem. So we can't merge as is then.

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jan 12, 2021

Yeah exactly. I'll move this back to draft state then re-request a review when it's ready!

@jsjoeio jsjoeio marked this pull request as draft January 12, 2021 17:14
@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jan 12, 2021

Fixed the color issues (H/T to @code-asher for pointing me in the right direction)

BEFORE
D01570C0-87A7-47D6-8CC8-7017963CEBA4

AFTER
E59F1722-7B11-4F6B-9A1A-F4518C55B4AE

@jsjoeio jsjoeio marked this pull request as ready for review January 12, 2021 17:43
@jsjoeio jsjoeio requested a review from nhooyr January 12, 2021 17:43
nhooyr
nhooyr approved these changes Jan 12, 2021
View reviewed changes
Copy link
Contributor

@nhooyr nhooyr left a comment

Choose a reason for hiding this comment

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

perfecto!! 🔥

I noticed that :visited isn't supported by any of VS Code's other UI links so it's fine that we don't here too.

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jan 12, 2021

Oh didn't even think about that - thanks for checking! Let's get this bad boy merged in!

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jan 12, 2021

After i fix the lint issues :)

@jsjoeio jsjoeio force-pushed the issue-2132-help-info-extension-search-view branch 2 times, most recently from 720a268 to 9d67fa1 Compare January 12, 2021 19:39
jsjoeio and others added 2 commits January 14, 2021 22:40
@jsjoeio @code-asher
feat: add helper header above extensions search
d950894 
Add a short message above the search box on the Extensions panel. This
helps explain the extension divergence to the user.

If they click dismiss, it stores an item in localStorage to prevent the
message from showing up on subsequent loads.

Co-authored-by: Asher <ash@coder.com>
@jsjoeio
fix: style links with correct colors
500ba92
@jsjoeio jsjoeio force-pushed the issue-2132-help-info-extension-search-view branch from 9d67fa1 to 500ba92 Compare January 14, 2021 22:41
@jsjoeio jsjoeio merged commit 3394ece into master Jan 14, 2021
@jsjoeio jsjoeio deleted the issue-2132-help-info-extension-search-view branch January 14, 2021 23:30
@jsjoeio jsjoeio mentioned this pull request Jan 14, 2021
Closed
@nhooyr nhooyr added this to the v3.8.1 milestone Jan 18, 2021
@dobladez dobladez mentioned this pull request Apr 27, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@code-asher code-asher code-asher approved these changes

@nhooyr nhooyr nhooyr approved these changes

Assignees

@jsjoeio jsjoeio

Labels
None yet
Projects
None yet
Milestone
v3.8.1
Development

Successfully merging this pull request may close these issues.
3 participants
@jsjoeio @nhooyr @code-asher