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

Blocked repositories page improvements #12438

Merged
merged 1 commit into from
Aug 26, 2022
Merged

Conversation

filiptronicek
Copy link
Member

@filiptronicek filiptronicek commented Aug 26, 2022

Description

Fixes some issues on inside https://github.com/gitpod-io/gitpod/blob/main/components/dashboard/src/admin/BlockedRepositories.tsx. Details are outlined below this comment.

Related Issue(s)

n/a

How to test

Release Notes

NONE

Documentation

Werft options:

  • /werft with-preview

@filiptronicek filiptronicek requested a review from a team August 26, 2022 12:20
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Aug 26, 2022
@@ -135,7 +135,7 @@ export function BlockedRepositoriesList(props: Props) {
</div>
<input
type="search"
placeholder="Search by URL Regex"
placeholder="Search by URL RegEx"
Copy link
Member Author

Choose a reason for hiding this comment

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

Made it so that we have the same capitalization everywhere

Copy link
Contributor

Choose a reason for hiding this comment

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

praise: Nice catch! ⚾

@@ -148,8 +148,10 @@ export function BlockedRepositoriesList(props: Props) {
</div>
</div>

<Alert type={"info"} closable={false} showIcon={true} className="flex rounded p-2 w-2/3 mb-2 w-full">
<span>Search entries by their Repositoriy URL regular expression (RegEx).</span>
<Alert type={"info"} closable={false} showIcon={true} className="flex rounded p-2 mb-2 w-full">
Copy link
Member Author

Choose a reason for hiding this comment

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

Here there was an extra class which was not used because w-full already changed the width.

Copy link
Contributor

Choose a reason for hiding this comment

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

praise: Thanks for noticing! 👀

<span>Search entries by their Repositoriy URL regular expression (RegEx).</span>
<Alert type={"info"} closable={false} showIcon={true} className="flex rounded p-2 mb-2 w-full">
<span>
Search entries by their repository URL <abbr title="regular expression">RegEx</abbr>.
Copy link
Member Author

Choose a reason for hiding this comment

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

Here besides fixing a typo, an abbreviation was added so that we both save space and utilize semantic HTML for abbreviations.

Copy link
Contributor

Choose a reason for hiding this comment

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

thought: Not particularly proud of how this is rendering by default (see screenshots below) or using this controversial abbreviation design pattern[1] as it comes with some accessibility concerns, but let's keep it for now until we have a better way to present this information. 🤝

Chrome Firefox Safari
a b c

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for introducing me to Microformats! It's a really great guide from what I'm seeing.

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-ft-blocked-repos-improvements.1 because the annotations in the pull request description changed
(with .werft/ from main)

@filiptronicek filiptronicek force-pushed the ft/blocked-repos-improvements branch from 4a6418e to 2f4bc9f Compare August 26, 2022 12:27
@roboquat roboquat added size/S and removed size/XS labels Aug 26, 2022
@@ -211,7 +213,7 @@ function AddBlockedRepositoryModal(p: AddBlockedRepositoryModalProps) {
setError("");
}, [p.blockedRepository]);

let save = (): boolean => {
const save = (): boolean => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Use const where possible

Copy link
Contributor

Choose a reason for hiding this comment

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

thought: Sounds legit and good, especially since @easyCZ from @gitpod-io/engineering-webapp already approved these changes. 🏓

@svenefftinge svenefftinge requested a review from gtsiolis August 26, 2022 12:28
Copy link
Member

@easyCZ easyCZ left a comment

Choose a reason for hiding this comment

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

LGTM. Adding hold in case you want @gtsiolis to review for UI.

/hold

@gtsiolis
Copy link
Contributor

gtsiolis commented Aug 26, 2022

Looking at this now! 👀

Copy link
Contributor

@gtsiolis gtsiolis left a comment

Choose a reason for hiding this comment

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

Thanks @filiptronicek for making this change, @svenefftinge for the ping[1], and @easyCZ for taking a look[2]! 🏀

LGTM!

Took longer to come back to this after today's tech talk (internal) because of some spaghetti I had to make. 🍝

@@ -135,7 +135,7 @@ export function BlockedRepositoriesList(props: Props) {
</div>
<input
type="search"
placeholder="Search by URL Regex"
placeholder="Search by URL RegEx"
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: Nice catch! ⚾

@@ -148,8 +148,10 @@ export function BlockedRepositoriesList(props: Props) {
</div>
</div>

<Alert type={"info"} closable={false} showIcon={true} className="flex rounded p-2 w-2/3 mb-2 w-full">
<span>Search entries by their Repositoriy URL regular expression (RegEx).</span>
<Alert type={"info"} closable={false} showIcon={true} className="flex rounded p-2 mb-2 w-full">
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: Thanks for noticing! 👀

@@ -211,7 +213,7 @@ function AddBlockedRepositoryModal(p: AddBlockedRepositoryModalProps) {
setError("");
}, [p.blockedRepository]);

let save = (): boolean => {
const save = (): boolean => {
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: Sounds legit and good, especially since @easyCZ from @gitpod-io/engineering-webapp already approved these changes. 🏓

<span>Search entries by their Repositoriy URL regular expression (RegEx).</span>
<Alert type={"info"} closable={false} showIcon={true} className="flex rounded p-2 mb-2 w-full">
<span>
Search entries by their repository URL <abbr title="regular expression">RegEx</abbr>.
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: Not particularly proud of how this is rendering by default (see screenshots below) or using this controversial abbreviation design pattern[1] as it comes with some accessibility concerns, but let's keep it for now until we have a better way to present this information. 🤝

Chrome Firefox Safari
a b c

@gtsiolis
Copy link
Contributor

/unhold

@roboquat roboquat merged commit 5080e98 into main Aug 26, 2022
@roboquat roboquat deleted the ft/blocked-repos-improvements branch August 26, 2022 22:45
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/S team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants