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

Responsive design #726

Closed
wants to merge 55 commits into from

Conversation

Projects
None yet
7 participants
@styki
Copy link
Contributor

styki commented Jul 7, 2016

I am working on the responsive design of the UI options as part of the Google Summer of Code program. This is the work that I have done for the past few weeks. I used the mock-ups (https://wiki.fluidproject.org/display/fluid/%28Floe%29+User+Interface+%28Learner%29+Options+Mobile+and+Responsive+Design) as a guideline for how it is supposed to look for small screens (max-width: 640px).

The changes are tested in Chrome and using the UI options demo. It is the only demo in which I have added all new files. There might be quite a few bugs in the other demos, which I will resolve later.

Known bug:
The change between small and large screen version is triggered only when the page is first loaded and everything is rendered. If you resize the window is changed, it does not switch to the other model. I think the reason for that is once the components are build, they become immutable, so I can't change the gradeNames.

I will be very thankful if you can give me some feedback on how to improve the implementation and a better way to construct the components if there is one.

@styki

This comment has been minimized.

Copy link
Owner Author

styki commented on 0f98bb1 Jun 29, 2016

The make and forget checks change the contextAwareness only when it is initialized. I think that the make and forgetChecks calls have to be made in a function invoked by a modelListener, but I still have not thought of an implementation.

@jobara jobara added the Review Only label Jul 13, 2016

}
});

fluid.defaults("fluid.button", {

This comment has been minimized.

Copy link
@jobara

jobara Jul 14, 2016

Member

The implementation seems to be for a specific type of button, one that increments/decrements a value. fluid.button seems like it should be for a generic button. This should probably be called something like fluid.stepper.button, fluid.button.stepper or something like that.

value: {
expander: {
funcName: "fluid.buttonCalculateValue",
args: ["{that}.options.incrementCoefficient", "{that}.options.buttonOptions.stepMultiplier", "{that}.model.value", "{that}.options.range.min", "{that}.options.range.max"]

This comment has been minimized.

Copy link
@jobara

jobara Jul 14, 2016

Member

The incrementCoefficient and stepMultiplier options should have default values.

changePath: "value",
value: {
expander: {
funcName: "fluid.buttonCalculateValue",

This comment has been minimized.

Copy link
@jobara

jobara Jul 14, 2016

Member

fluid.buttonCalculateValue should be an invoker as well.

@@ -802,6 +948,26 @@ var fluid_2_0_0 = fluid_2_0_0 || {};
}
});

fluid.defaults("fluid.prefs.blueColorFilter.textfieldButtons", {

This comment has been minimized.

Copy link
@cindyli

cindyli Aug 15, 2016

Member

Use the common grade created on buttons for text size and line space panels.

@@ -299,7 +299,29 @@ var fluid_2_0_0 = fluid_2_0_0 || {};
var attrs = {height: height + 15}; // TODO: Configurable padding here
var panel = separatedPanel.slidingPanel.locate("panel");
panel.css({height: ""});
iframe.animate(attrs, 400);
if ($(window).width()< "640") {

This comment has been minimized.

Copy link
@cindyli

cindyli Aug 15, 2016

Member

No hardcoded 640. Use component option instead.

iframe.removeAttr("style");

//The rest of the if make the scroll go to the next tool
var jqueryDokkument = $(dokkument);

This comment has been minimized.

Copy link
@cindyli

cindyli Aug 15, 2016

Member

Move this special handling into its own utility function.

jqueryDokkument.on("scroll", function () {
var scrollPosition = jqueryDokkument.scrollTop();
//Scroll down
if (lastPosition > scrollPosition) {

This comment has been minimized.

Copy link
@cindyli

cindyli Aug 15, 2016

Member

Add code comments to clarify the purpose of these calculation.

options: {
gradeNames: "fluid.prefs.prefsEditorConnections",
model: {
lineSpace: "{prefsEditor}.model.preferences.blueColorFilter"

This comment has been minimized.

Copy link
@cindyli

cindyli Aug 15, 2016

Member

typo of "lineSpace"?

options: {
gradeNames: "fluid.prefs.prefsEditorConnections",
model: {
toc: "{prefsEditor}.model.preferences.muteAudio"

This comment has been minimized.

Copy link
@cindyli

cindyli Aug 15, 2016

Member

typo of "toc"?

@@ -398,6 +452,7 @@ var fluid_2_0_0 = fluid_2_0_0 || {};
textSize: "%messagePrefix/textSize.json",
textFont: "%messagePrefix/textFont.json",
lineSpace: "%messagePrefix/lineSpace.json",
blueColorFilter: "%messagePrefix/blueColorFilterjson",

This comment has been minimized.

Copy link
@cindyli

cindyli Aug 15, 2016

Member

missing dot in from of "json". Please test to make sure messages are properly loaded and rendered. Thanks.

This comment has been minimized.

Copy link
@styki

styki Aug 17, 2016

Author Contributor

I fixed it, but everything worked fine before that, I don't know how and why.

@@ -21,11 +21,41 @@ var fluid_2_0_0 = fluid_2_0_0 || {};
* contrast, table of contents, inputs larger and emphasize links
*******************************************************************************/

var check;
var widthCheck = function() {

This comment has been minimized.

Copy link
@cindyli

cindyli Aug 15, 2016

Member

Avoid using private variables and functions. Use a global namespace starting as "fluid.prefs.xxxx" for this function. Worthwhile to use context awareness for this check too.

This comment has been minimized.

Copy link
@cindyli

cindyli Aug 15, 2016

Member

Move the check related code into a separate .js file. Stick with starter schema handling in this file.

This comment has been minimized.

Copy link
@styki

styki Aug 22, 2016

Author Contributor

I tried encapsulating the check in a function and the use a expander, but I got and error. For now I will leave it like that. If you have a better I idea you can try it, because I don't know which will be the best solution for this case.

jQueryUI: {
contextValue: "{fluid.prefsWidgetType}",
equals: "jQueryUI",
gradeNames: ["fluid.prefs.auxSchema.starter.textSize.jQueryUI", "fluid.prefs.auxSchema.starter.lineSpace.jQueryUI", "fluid.prefs.auxSchema.starter.blueColorFilter.jQueryUI"]

This comment has been minimized.

Copy link
@cindyli

cindyli Aug 15, 2016

Member

Keep in mind UIO is flexible to mix in any panels. Having all three panel grades combined together would cause issue when some or all of these panels are not requested to compose UIO. These grades to be connected individually with their own starter components.

This comment has been minimized.

Copy link
@styki

styki Aug 17, 2016

Author Contributor

There was a big problem with the contextAwareness and this was the only solution that worked.

This comment has been minimized.

Copy link
@cindyli

cindyli Aug 17, 2016

Member

Please refer the existing code in the master branch of how different versions of sliders are attached on the text size and line spacing panels - https://github.com/fluid-project/infusion/blob/master/src/framework/preferences/js/StarterSchemas.js#L42, as well as line 101.

This comment has been minimized.

Copy link
@styki

styki Aug 17, 2016

Author Contributor

I tried to do that, but the problem is that the we must have the responsive check first and if it fails then we have to check for jQuery or native slide. In each case we had a different template to be applied and they did not override each other. There was also a problem with nesting contextAwarenesses. I tried I lot of different versions and this is the only one that worked.

This comment has been minimized.

Copy link
@cindyli

cindyli Aug 17, 2016

Member

Instead of nesting contextAwarenesses, have you tried to narrow down the context checks into 4 scenarios:

  • jQuery + non-mobile
  • jQuery + mobile
  • nativeHtml + non-mobile
  • nativeHtml + mobile

This comment has been minimized.

Copy link
@styki

styki Aug 22, 2016

Author Contributor

I make a little bit more testing in the upcoming days, but there are to many things connected and will leave it like that for now.

target: "{that > fluid.stepper.button}.options",
record: {
model: "{fluid.textfieldButtons}.model",
range: "{textfieldButtons}.options.range",

This comment has been minimized.

Copy link
@cindyli

cindyli Aug 15, 2016

Member

It would be nice to be consistent with other lines by using {fluid.textfieldButtons}.

@the-t-in-rtf

This comment has been minimized.

Copy link
Contributor

the-t-in-rtf commented Oct 12, 2016

@cindyli @jhung, what is the status of this? Is it likely to be merged? I have an otherwise responsive design that needs to be integrated with UIOptions, and would like to use a responsive version if it's ready or heading toward being ready.

@cindyli

This comment has been minimized.

Copy link
Member

cindyli commented Oct 14, 2016

@the-t-in-rtf, thanks for bringing this up.

This pull request contains three things:

  1. The responsive design that replaces sliders with text field buttons on mobile devices;
  2. A new feature called "Blue Color Filter";
  3. Another new feature called "Mute Audio".

Before this pull request can be merged into the master, what's left to do:

  1. The CSS part of the responsive design needs to be reviewed by @jhung;
  2. The component for creating text field buttons and new features (item 2, 3 on the above list) need tests to be written.

@jhung, please add if I missed anything above.

I'm not sure how mature it is that new features of "Blue Color Filter" and "Mute Audio" have been decided to be included as UIO default panels. (@jhung, do you have some ideas?) If it's not yet decided, we probably should consider to split this pull request to 2 or even 3 individual pull requests so the responsive design itself can make into the master sooner and easier.

jobara added a commit to jobara/infusion that referenced this pull request Jan 19, 2017

@mgifford

This comment has been minimized.

Copy link

mgifford commented Mar 19, 2018

Would love to see this as part of a new release of this library.

@waharnum

This comment has been minimized.

Copy link
Member

waharnum commented Mar 19, 2018

@mgifford if you're referring to the responsive layout specifically, we've recently released a responsive version of the Preferences Framework that you can see in action in the demo at https://build.fluidproject.org/infusion/demos/prefsFramework/, among other places

@mgifford

This comment has been minimized.

Copy link

mgifford commented Mar 19, 2018

Yes.. That's it. Is the code for that up in Git? This was the closest I could find to an open issue queue about this issue.

@waharnum

This comment has been minimized.

Copy link
Member

waharnum commented Mar 19, 2018

@mgifford Yes, the latest master version of Infusion should include responsive layout for the Preferences Framework out of the box.

@amb26

This comment has been minimized.

Copy link
Member

amb26 commented Mar 23, 2019

@cindyli, @waharnum - is there work we need to rescue from this pull or is everything incorporated in the 2018 release with responsive design?

@waharnum

This comment has been minimized.

Copy link
Member

waharnum commented Mar 25, 2019

@amb26 I don't believe so, but @cindyli should confirm.

@cindyli

This comment has been minimized.

Copy link
Member

cindyli commented Mar 25, 2019

Most in this pull request was covered by the 2018 release with responsive design. The rest about the new "Blue Color Filter" feature has been taken off the implementation list I believe.

Closed.

@cindyli cindyli closed this Mar 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.