Opened 9 years ago
Closed 8 years ago
#15778 closed feature (fixed)
Support for textDir property for dojox.mobile widgets
Reported by: | sashash | Owned by: | Eric Durocher |
---|---|---|---|
Priority: | undecided | Milestone: | 1.9 |
Component: | DojoX Mobile | Version: | 1.7.3 |
Keywords: | Cc: | cjolif | |
Blocked By: | Blocking: |
Description
Attachments (7)
Change History (29)
comment:1 Changed 9 years ago by
Summary: | Support for textDir property for dojox.mobiule widgets → Support for textDir property for dojox.mobile widgets |
---|
Changed 8 years ago by
Attachment: | mobile_bidi_tests1.patch added |
---|
Changed 8 years ago by
Attachment: | mobile_bidi_tests2.patch added |
---|
Manuel tests (part 2) for bidi support for dojox/mobile - Thanks Helena Halperin (IBM, CCLA), Alex Shensis (IBM, CCLA), Amir Brandsdorfer (IBM, CCLA)
Changed 8 years ago by
Images used in manuel tests or bidi support for dojox/mobile - Thanks Helena Halperin (IBM, CCLA), Alex Shensis (IBM, CCLA), Amir Brandsdorfer (IBM, CCLA)
Changed 8 years ago by
Images used in manuel tests or bidi support for dojox/mobile - Thanks Helena Halperin (IBM, CCLA), Alex Shensis (IBM, CCLA), Amir Brandsdorfer (IBM, CCLA)
Changed 8 years ago by
Images used in manuel tests or bidi support for dojox/mobile - Thanks Helena Halperin (IBM, CCLA), Alex Shensis (IBM, CCLA), Amir Brandsdorfer (IBM, CCLA)
comment:2 Changed 8 years ago by
Cc: | cjolif added |
---|
First patch contains the library code + doh tests, next 2 patches contain manual tests. Images must be put in dojox/mobile/tests/images.
comment:3 Changed 8 years ago by
Milestone: | tbd → 1.9 |
---|---|
Type: | defect → feature |
comment:4 Changed 8 years ago by
Looking more at the patch I still have two little additional comments:
- I think we could even get rid of the name of the non-bidi class indeed it will never be used externally, so we should just able to pass null here instead of "NonBidi?..."? If there is no objection I can do the change myself before committing.
- for the has flag naming, before committing I will double check we are correctly naming it.
comment:5 Changed 8 years ago by
Have you tried this code with the API doc parser? I doubt it works. Hopefully I'm wrong.
comment:6 Changed 8 years ago by
does declare add any value for simple object constructors over plain function definition, e.g. declare(null, {...
comment:7 Changed 8 years ago by
@bill: good point, we'll check that, thanks.
@peller: not sure to get it, sorry... can you clarify?
comment:8 Changed 8 years ago by
Just a general question, and not a critical one. Objects without inheritance could be constructed simply without declare. Using declare is consistent but arguably wasteful (not that wasteful... you're pulling it in anyways) Might be worth it for consistency and necessary for documentation. Nevermind.
comment:9 Changed 8 years ago by
@bill:
I tried to run the doc parser on the patched code, the result is basically the same as without the patch, except that the bidi/* classes now appear. The methods added by the bidi/* extension classes do not appear in the doc, I suppose this is because the parser ignores the has("bidi") case... But, since the added/replaced methods are either private attribute setters or overridden widget methods, there would be very little value in documenting them. Basically, the purpose of this extension is to correctly support the textDir attribute on all widgets, and this attribute is already documented.
So all in all my opinion is that the patch is OK from the API doc perspective. Other opinions?
Side question: if we really wanted to document the extensions, is there something like staticHasFeatures for the doc parser?
comment:10 Changed 8 years ago by
OK, glad to hear the doc is still working. Since the mixins in the bidi/ directory will show up in the API doc, you might want to improve the comments to make it clear that users aren't supposed to require those modules directly. And to fix the repeated misspelling of "Caracters".
About staticHasFeatures , the doc parser has a environmentConfig setting in config.js but I don't know the details about it.\
BTW, it would be easy to implement BIDI mobile widgets as subclasses of the original dojox/mobile widgets. Then you wouldn't need to touch the original files. Unfortunately that would put the burden on the user to know to use the new subclasses instead of the old ones. I couldn't find a way to get the loader to make that transparent, without a long aliases mapping table.
Changed 8 years ago by
Attachment: | mobile_bidi.patch added |
---|
Bidi support for dojox/mobile - Thanks Helena Halperin (IBM, CCLA), Alex Shensis (IBM, CCLA), Amir Brandsdorfer (IBM, CCLA)
comment:11 Changed 8 years ago by
New version of main mobile_bidi.patch with corrections in doc comments of bidi/* classes (formatting, missing dots, spelling).
comment:16 Changed 8 years ago by
Code/tests committed. Will be closed once documented/release noted.
comment:17 Changed 8 years ago by
I setup #16489 to follow the same approach for dijit. Not necessarily creating a shadow file for every single widget, but at least supporting the "dojo-bidi" has() flag, and separating bidi support code into a separate section in each file.
comment:19 Changed 8 years ago by
Status: | new → assigned |
---|
Changed 8 years ago by
Attachment: | mobile_tab.patch added |
---|
Cleanup: replace spaces by tabs + removed some spaces/tabs - Helena Halperin (IBM, CCLA)
comment:22 Changed 8 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Manuel tests (part 1) for bidi support for dojox/mobile - Thanks Helena Halperin (IBM, CCLA), Alex Shensis (IBM, CCLA), Amir Brandsdorfer (IBM, CCLA)