-
Notifications
You must be signed in to change notification settings - Fork 162
Fix breakpoints on Projects show page #4829
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
base: master
Are you sure you want to change the base?
Conversation
| @media (min-width: 1600px) { | ||
| left: 18.5vw; | ||
| width: 30.5vw; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A possible alternative to the magic numbers here would have us move the job-info pane outside of the pill div to anchor it to the div.pill-box. However doing this requires us to customize the default popup behavior, and compute the height offset to correctly place the popup. Given how much more complex it is to do any of that with custom javascript, the approach taken here is an intentional compromise. I would also be open to adding system tests to ensure that the popup never overflows the jobs container, (similar to some of the positional testing we will have to do with workflows) but that should wait until after 4.1
|
Also @johrstrom did you want some javascript to make sure one job popup opens at a time? I initially avoided it for complexity's sake but if you feel it is necessary to make it less confusing I would be happy to add something. Obviously some javascript is more complex than none, but I don't foresee the actual code being too bad. |
May be easier and better if they just didn't overlap. That is if you can visually see that they're stacked on top of each other then the end user can see what's going on. Which now actually makes me wonder how accessible this is, if you'd even be able to focus and read two or more at a time. |
Not sure how we could do that via css alone, I feel like the only way to add slight offsets (if I understand you right) would be via javascript, which would still need to know how many popups are open (in a certain row) and what the initial height offset was, which seems more complicated than just closing open popups when a new popup is opened. If you have something in mind I would totally be open to it, but at first consideration it seems harder than the 'enforce one at a time' approach
I just checked this out with a screenreader and yeah it isn't that accessible at the moment. The main issues were
For the first point, this is something we will have to figure out for 4.2 and apply to all the custom I also think the accessibility angle is another reason for 'one at a time' operation here, since while we have made it possible to close things by changing the text color on opened jobs, this is a purely visual cue. However if you are navigating by screenreader you also are not limited in opening jobs obscured by a different popup, so maybe this isn't as much of a concern. That is, you can press 'enter' on any focused job and the job details will be read, but as long as you don't need to see the obscured jobs, there is no reason to ever close a popup. |
Fixes #4773 and fixes #4777.
This solves the issue of job pills overlapping by removing the 'completed' text from completed job pills (this is redundant due to the 'Completed Jobs' header) and using flex positioning in place of rigid columns to ensure that whatever size the jobs panel is, the individual jobs divide into the correct number of columns to prevent overlap. Since jobs naturally pile up in the completed section, the removal of the redundant text immediately saves a lot of space on the page.
This created an issue with the job info popup, which changed the apparent width of its parent, refreshing the flex display and throwing off all the other jobs (which were rendering below the job info popup). I solved this issue as well as that of the jobs info pane being poorly formatted (depending on where the job pill is) by making the info pane absolute, and centering it within the jobs pane. This way the info pane positions independently of its direct parent, making it consistent between different jobs. It also does not affect the display of any job pills.
We position the job info pane using pure CSS, approximating the position of the job pane using precise % values. Because the arrangement changes at different breakpoints, we set the positioning separately at each breakpoint. As the display grows larger, the gaps in our approximation begin to add up, so we add one more conditional css clause at 1600px, which is able to correctly display past 3500px. Past that the info pane will not be perfectly centered, but this is such a minority of uses that it isn't worth fine-tuning any further (it never leaves the job pane, just goes off center slightly). While a more complex expression might be able to correctly position ad infinitum, this approach seems to strike a balance between simplicity and accuracy within the specific breakpoints. (I recommend testing this aspect by making the window as narrow as possible, opening a job info pane, and watching it reposition as you increase the window width).
Because I had to determine the position of the job info pane for each breakpoint, I also resolved all of the latent breakpoint issues I had noticed in #4777, adding a
div#project-jobs-files-colto allow the jobs pane and files pane to stack at medium breakpoints (leaving a two column layout) before finally collapsing into a single column at the small breakpoint.There are still some slight issues with the files pane at certain breakpoints, but this will have to be covered in #3930 when we figure out how to add an action button to the files display.
Lastly, I ensured that job pills stay highlighted while the info pane is open, and that job pills format correctly throughout the javascript operations, by collecting the job pill formats into a single css class. This was especially necessary because while job info panes cannot visibly overlap anymore, they can perfectly overlap and create a confusing scenario. However with the highlighting you can see which jobs are opened at a given time so that you can close them. Future fixes to this issue would have to include disabling all the other pills when one is opened, but that is likely too complex to implement at the moment.