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

fix: show all envs in project tables unless you've explicitly hidden some #6812

Merged
merged 9 commits into from
Apr 10, 2024

Conversation

thomasheartman
Copy link
Contributor

@thomasheartman thomasheartman commented Apr 10, 2024

This PR changes the behavior of the project tables' environment columns based on input from customers.

Up until now, you have been shown either the first project or the first three projects in the list of the project's environment. The decision on whether to show one or three is based on screen size. The breakpoint appears to be about 1280px. Above that you get three, below it you get one.

With this PR, we'll show you all environments by default, regardless of screen size. However, that's just for the default values. If you manually change column visibility, those changes will of course be respected.

I've used a new package, css-mediaquery, to test that all screen sizes show all envs.

Copy link

vercel bot commented Apr 10, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
unleash-monorepo-frontend ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 10, 2024 7:43am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
unleash-docs ⬜️ Ignored (Inspect) Visit Preview Apr 10, 2024 7:43am

Comment on lines +6 to +25
import mediaQuery from 'css-mediaquery';

const createMatchMedia = (width: number) => {
return (query: string) => {
return {
matches: mediaQuery.match(query, { width }),
media: '',
addListener: () => {},
removeListener: () => {},
onchange: () => {},
addEventListener: () => {},
removeEventListener: () => {},
dispatchEvent: () => true,
};
};
};

const resizeScreen = (width: number) => {
window.matchMedia = createMatchMedia(width);
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This bit allows us to resize the screen in the tests (based on this article that I found). I'm wondering whether we should extract it into a separate file or leave it here for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the actual fix to the problem.

Copy link
Member

@Tymek Tymek left a comment

Choose a reason for hiding this comment

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

Since it's the first time we're doing this I think it's OK to not have a 'central/unified' utility for it. But please double-check if we are testing all relevant states of the screen, or "all greater then breakpoints"

resizeScreen(
theme.breakpoints.values[
screenSize as keyof typeof theme.breakpoints.values
] + 1,
Copy link
Member

Choose a reason for hiding this comment

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

I suspect we have an off-by-one issue here. If you cut a stick in 2 places, how many sticks you will end up with? It's the same with breakpoints.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a fair point. But I had in fact thought of this. The thing is, assuming I'm looking at the right file (frontend/src/themes/theme.ts), then breakpoints is defined as such:

    breakpoints: {
        values: {
            xs: 0,
            sm: 600,
            md: 960,
            lg: 1280,
            xl: 1536,
        },
    },

So in this case, the smallest size we set for the screen is 1 pixel. The way I read this, xs is from 0 px and up, sm from 600 px and up etc. Thus, using one px above their lower threshold should put us safely within their zone.

Am I making sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or, the breakpoint values are defined by their lower bound.

Copy link
Member

Choose a reason for hiding this comment

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

O, perfect, thanks for clarification

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory, we could probably skip the +1, but that just means we avoid any weird edge cases if we use strictly vs greater than or equal to

@thomasheartman thomasheartman merged commit c9beb86 into main Apr 10, 2024
13 checks passed
@thomasheartman thomasheartman deleted the feat/show-all-envs-in-proj branch April 10, 2024 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants