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

Shrinking 2 split areas to a width of 0 prevents you from being able to open them #213

Open
unre4l opened this issue Nov 5, 2019 · 2 comments · May be fixed by #319
Open

Shrinking 2 split areas to a width of 0 prevents you from being able to open them #213

unre4l opened this issue Nov 5, 2019 · 2 comments · May be fixed by #319

Comments

@unre4l
Copy link

unre4l commented Nov 5, 2019

As i am also interested in this phenomenon regarding Split Grid, i felt free to reference the issue by @ChuckFields here.

original issue: stijlbreuk-dev/vue-split-grid#9, gif by @ChuckFields

https://github.com/stijlbreuk/vue-split-grid/issues/9

@nino-vrijman already did some invesitgation (stijlbreuk-dev/vue-split-grid#9 (comment))

This seems to be an upstream bug in Split Grid . When dragging a gutter Split Grid takes care of setting either the grid-template-columns or grid-template-rows prop on the SplitGrid component from this module.

See this forked CodePen. When dragging the second gutter to one end of the grid untill you have 2 SplitGridArea's with a size of 0 the gridTemplateStyle will be set correctly to something like 0fr 5px 0fr 5px 100fr (see the console). If you then try to drag the second gutter to the right again Split Grid will try to set the grid-template-* CSS property to something like 0fr 5px Infinityfr 5px Infinityfr which seems to be invalid CSS, you can confirm this by watching the output of the the onDrag event (which is the raw onDrag event emitted by the Split Grid library). I suggest you submit an issue in the original library's repo.

Following the forked CodePen, my chrome outputs similiar NaNfr 5px NaNfr 5px 226.8fr, when trying to open the grid again.

Could this be a bug in Split Grid?

@johnsusek
Copy link

Put a PR in for this, fixed version here https://github.com/johnsusek/split

@rassie
Copy link
Contributor

rassie commented Sep 3, 2020

I think the problem lies deeper than the fix in the linked PR suggests.

Let's look at the code for percentages (same arguments are also valid for fr):

    const trackPercentage = this.trackValues.filter(
        track => track.type === '%',
    )

    [...]

    if (trackPercentage.length) {
        const track = firstNonZero(trackPercentage)

        if (track !== null) {
            this.percentageToPixels =
                this.computedPixels[track].numeric /
                trackPercentage[track].numeric
        }
    }

In this snippet, the variable track is the index of the first non-zero percentage value. However, this index is based on the trackPercentage, which is a filtered array. For example, if my grid-template-columns is 0% 0px 1fr 10px 30% (collapsed left sidebar, open right sidebar and auto-width main area), this gets filtered to [0, 30], of which the first non-zero value is 30, i.e. index 1. However, index 1 is also used to fetch a pixel value from this.computedPixels, which contains all five tracks and thus, index 1 in my example is 0px and is not the pixel value for 30% (correct index would have been 4).

Further down:

    } else if (this.trackValues[this.bTrack].type === '%') {
        const targetPercentage = bTrackSize / this.percentageToPixels
        this.tracks[this.bTrack] = `${targetPercentage}%`
    }

Since this.percentageToPixels is now 0, targetPercentage is Infinity, which is the basic symptom in this issue. The core problem is using invalid indices to figure out pixel values. The problem appears every time the position of the first non-zero value in either fr or % category corresponds to a zero value in the full dimensions list, i.e. 0% 0px 1fr 10px 30% is a problem, because 30% corresponds to 0px, 0fr 5px 0fr 5px 100fr is a problem because 100fr corresponds to the second 0fr and so on.

@nathancahill could you by any chance take a look at this? I might be able to prepare a PR, but would be happy to avoid working with a fork. Explanations why the above is wrong are also greatly appreciated :)

rassie added a commit to rassie/split that referenced this issue Sep 3, 2020
rassie added a commit to rassie/split that referenced this issue Sep 3, 2020
RonaldTreur added a commit to RonaldTreur/split that referenced this issue Nov 10, 2020
…fail

Instead of using a trackIndex (track) retreived by searching for a non-zero value in a filtered (potentially smaller) array, get the index from the actual trackValues array and skip creating filtered array variants

Additionally:
* remove redundant outer `if`-blocks and associated computations, surrounding the fraction/percentage `..ToPixel`-calculations
* expand utility tests to fully test updated `firstNonZero` function
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants