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

Tech flexbox reflow #388

Merged
merged 22 commits into from
Sep 12, 2018
Merged

Tech flexbox reflow #388

merged 22 commits into from
Sep 12, 2018

Conversation

alastc
Copy link
Contributor

@alastc alastc commented Jun 28, 2018

New technique for reflow, you can preview Flexbox reflow, and there is an associated example page.

@alastc alastc added Techniques Ready for initial review A new technique ready for +1s or itterations labels Jun 28, 2018
@mraccess77
Copy link

Same issues for grid reflow would apply to flexbox reflow regarding clarity of testing the horizontal and vertical aspects separately and how the requirements only apply to content scrolling in that direction and the need to set a starting viewport width before zooming 400%.

- vertical / horizontal text separated
- Link to CSS spec added
- Code example updated
- Test procedure vertical / horizontal text separated
@jake-abma
Copy link
Contributor

Hi @mraccess77, @alastc,

I've made some updates based on #387, you can see them in this diff.

@alastc , how can we show a live example when pull request is not processed?

@jake-abma
Copy link
Contributor

changes:

vertical / horizontal separation

  • vertical / horizontal text separated
  • Link to CSS spec added
  • Code example updated
  • Test procedure vertical / horizontal text separated

@mbgower
Copy link
Contributor

mbgower commented Jul 25, 2018

I think this is a perfect candidate for a failure technique:

NOTE: Flexbox has the possibility to cause a keyboard navigation disconnect by using the order and reverse properties. The CSS Flexible Box Layout module warns against resequencing content logic as they cause incorrect source ordering and are non-conforming.

@mbgower
Copy link
Contributor

mbgower commented Jul 25, 2018

+1, with a minor editorial change: For step 4 of both test procedures, a slight editorial change from "whether" to "that". Rationale is that the test processes lead to a result, that is then tested to be true.

Check whether all content and functionality is available without horizontal scrolling.

becomes

Check that all content and functionality is available without horizontal scrolling.

(with same wording change in the second procedure covering vertical)

@jake-abma
Copy link
Contributor

Hi @mbgower, thx for the inspect and suggestions, after my vacation will do the changes and techniques. Cheers!

@allanj-uaag
Copy link
Contributor

+1

@awkawk awkawk requested a review from lauracarlson August 21, 2018 15:51
@lauracarlson
Copy link
Contributor

+1

@allanj-uaag
Copy link
Contributor

allanj-uaag commented Aug 22, 2018 via email

Copy link
Contributor

@lauracarlson lauracarlson left a comment

Choose a reason for hiding this comment

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

+1

@alastc alastc removed the Ready for initial review A new technique ready for +1s or itterations label Aug 22, 2018
@jake-abma
Copy link
Contributor

jake-abma commented Aug 24, 2018

@mbgower, the last part (4.) of the procedure suggests no scrolling is allowed at all, so we'll need to be clear on the separation. If we are not going to split the two up at least I think we should separate them in another sentence. After some re-reading the previous / first suggestion would be fine after all with the only adjustment to replace "whether" to "that" resulting in:

Same suggestion for #387 Any objections?

Procedure

  1. Display content in a user agent capable of 400% zoom with a viewport-width of 1280 CSS pixels.
  2. If the text is read horizontally: Zoom in by 400%.
  3. Check that all content and functionality is available without horizonal scrolling.
  4. If the text is read vertically: Set the viewport height to 256 CSS pixels.
  5. Check that all content and functionality is available without vertical scrolling.

NB: If the browser is not capable of zooming to 400%, you can reduce the width of the browser proportionally. For example, at 300% zoom the viewport should be sized to 960px wide.

Expected Results

Check #3 is true.
Check #5 is true.

@jake-abma
Copy link
Contributor

Replaced the separation

Tests

Procedure 1
If the text is read horizontally
Display content in a user agent capable of 400% zoom with a viewport-width of 1280 CSS pixels.
Zoom in by 400%.
Check whether all content and functionality is available without horizontal scrolling.

NB: If the browser is not capable of zooming to 400%, you can reduce the viewport width of the browser proportionally. For example, for 300% zoom set the viewport width at a equivalent to 960 CSS pixels.

Expected Results
Check #4 is true.

Procedure 2
If the text is read vertically.
Display content in a user agent capable of 400% zoom with a viewport-height of 1024 CSS pixels.
Zoom in by 400%.
Check whether all content and functionality is available without vertical scrolling.

NB: If the browser is not capable of zooming to 400%, you can reduce the viewport height of the browser proportionally. For example, for 300% zoom set the viewport height at a equivalent to 768 CSS pixels.

Expected Results
Check #4 is true.

This is a sufficient technique for a success criterion, failing this test procedure does not necessarily mean that the success criterion has not been satisfied in some other way, only that this technique has not been successfully implemented and can not be used to claim conformance.

jake-abma and others added 3 commits August 24, 2018 10:35
Removed the applicability content (assiming it defaults to CSS).
Removed the section on complex layouts.
Added resource links.
Converted note to class=note.
Added test procedure from the grids example.
Copy link
Member

@michael-n-cooper michael-n-cooper left a comment

Choose a reason for hiding this comment

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

This looks ok from a source code readiness perspective. I'm a little unsure about the "NB", I'm inclined to say it should be just a note (with class="note"), but if it's a different semantic we think we'll introduce, perhaps we should create a class and generator support for it? As is it sticks out to me, but not enough to not approve the technique for the moment.

@alastc
Copy link
Contributor Author

alastc commented Aug 30, 2018

Ach, that wasn't intentional, I have a severe muscle-memory for "NB" I'm trying to train myself out of! Converted it.

@alastc
Copy link
Contributor Author

alastc commented Sep 5, 2018

NB: I moved the example into this branch in preparation for publication.

Copy link
Member

@michael-n-cooper michael-n-cooper left a comment

Choose a reason for hiding this comment

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

I pushed a commit to standardize working-example link and expected results, and link to related technique also in the queue

@alastc alastc merged commit 70f4373 into master Sep 12, 2018
@alastc alastc deleted the tech-flexbox-reflow branch September 12, 2018 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants