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.

Sign up for GitHub

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

Jump to bottom

Merge Branch: Rtl language support and computed css #2135

Closed
wants to merge 22 commits into from
Closed

Merge Branch: Rtl language support and computed css #2135

wants to merge 22 commits into from

Conversation

simonberger
Copy link
Member

No description provided.

simonberger and others added 17 commits March 8, 2020 00:07
@simonberger
#712 Added support for languages read from right to left
1f8f057 
- Text aligned right when direction is set to rtl
- Text of RTL languages is reordered while LTR languages should be untouched
@simonberger
Fixed set align:right through direction:rtl just if no align has been…
2dc98a1 
… set
@simonberger
fixed last commit
30abbdb
@simonberger
#2107 #712 Activated shaping (joining of glyphs) for languages like a…
62e7b36 
…rabic
@bsweeney
Update Style class style property dependency map
dde0061
@bsweeney
Organize Style class properties by CSS version
b09cbff
@bsweeney
Set default ref size to the current font size
939483d 
Calculations based on font size should be calculated based on the current font size, not the default.
@bsweeney
Tweak logic for Style class generic getter/setter
ffd1728 
- Only set the computed property on fallback to the passed-in value if it is not blank or "inherit".
- When getting a property, only set it to the default if there is no specified value
- When getting a property, call the setter with the specified value if there is no computed value
@bsweeney
Support computed style value for font properties
a843be2
@bsweeney
Call standard style property getter/setter for vendor properties
3b3242e
@bsweeney
Support computed style value for spacing properties
c7af941 
- word-spacing
- letter-spacing
- line-height
@bsweeney
Support computed style value for background properties
f1bcdaf 
- background
- background-attachment
- background-image-resolution
- background-position
- background-repeat
- background-size
@bsweeney
Support computed style value for border properties
3ad315e 
- border (color, style, width for each side)
- border-radius (for each corner)
- border-spacing
- outline (color, style, width)

This includes generic helpers utilized by the getters/setters:
- _get_width
- _set_border_radius_corner
- _set_style_side_type
@bsweeney
Support computed style value for other properties
2080444 
- color (and helper methods)
- image-resolution
- list-style (list-style-image)
- page-break-after
- page-break-before
- transform
- transform-origin
- z-index
@simonberger
Merge branch 'css-computed-props' into feature/rtl_language_support
deb065b
@simonberger
Revert "Fixed set align:right through direction:rtl just if no align …
c59f990 
…has been set" and "fixed last commit"

This reverts commit 2dc98a1
This reverts commit 30abbdb
@simonberger
Earlier not commited state
d1af531
@simonberger simonberger mentioned this pull request Apr 12, 2020
Merged
@simonberger
Copy link
Member Author

simonberger commented Apr 12, 2020 •
edited by bsweeney

@bsweeney I tested your computed props changes and tried to get the text align right working via direction:rtl as well as direction:right but a text-align: left on a parent element. It is the same scenario I had some weeks before.

I did not manage to get both scenarios working without code changes

First with text-align inheritance

<body style="text-align: left"">
	<div style="direction: rtl"><p>
		test
	</p></div>
</body>

Second without

<body>
	<div style="direction: rtl"><p>
		test
	</p></div>
</body>

If I try to set the text-align: right in the set_direction method Scenario 1 works but inheritance will be destroyed as this will be called later.

If I instead try to return text-align right in a new get_text_alignmethod when direction right is set and no inherited or directly set text-align is there I have no chance to detect it. I do not know how a set_text_align method could be created to avoid those problems.
Summed up I have the same problems as before. ;(

@bsweeney
Add text-align setter to the Style class
20f4979 
Getter is excluded under the assumption that the user is hooking into the generic getter/setter, which will evaluate to the correct value if no value is set. Per the spec: a nameless value that acts as 'left' if 'direction' is 'ltr', 'right' if 'direction' is 'rtl'.
@bsweeney
Fix Style class letter and word spacing setters
d876116
@bsweeney
Improve Style class text-align setter logic
5c6f75a 
- add keyword check
- add dependency on direction
@bsweeney
Copy link
Member

I reviewed the status of the code and realized I had a missed a few issues:

  • when I moved the letter/word spacing logic from the getter to the setter I didn't quite fill out the logic correctly
  • need to validate that the passed-in value for text-align is a valid keyword
  • text-align is dependent on direction and so needed to specify that relationship in the dependency map

Try with the current rev of that branch and see if it helps.

@simonberger
Copy link
Member Author

Ok then there seems no other way around it. Yes it works like this.

@bsweeney
Copy link
Member

Thoughts on how to proceed then? Complete review on this PR and merge, close the other PRs?

@simonberger
Copy link
Member Author

I'd like to close this merge request. Instead my plan is to wait until your changes can be merged into develop and merge develop into the RTL branch where I repeat the necessary small changes.

@simonberger simonberger mentioned this pull request Apr 20, 2020
Open
@bsweeney
Copy link
Member

OK, I've merged the computed property branch.

@simonberger
Copy link
Member Author

simonberger commented Apr 26, 2020 •
edited

Ok I'll finalize the merge request next week. Beside some minor review comments I found an endless recursion problem in your merge request.

Edit: I wrote them weeks before but now noted I forgot to submit the review...
Edit 2: Still submitted them now also that merge request is closed.

@bsweeney
Copy link
Member

ah ha ha that's why those few comments were on an older commit. Anyway, I responded, take a look and let me know if you have any more thoughts. I'll commit a few changes and a new PR once we resolve current discussions.

@simonberger
Copy link
Member Author

It was good. Otherwise I maybe never would have noticed my fail and you wait for my approval and I wait the fixes. ;)

@simonberger
Copy link
Member Author

I'll wait your changes before I merge develop.

@simonberger
Copy link
Member Author

Closed as discussed. It was only a test branch.

@simonberger simonberger closed this May 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@simonberger @bsweeney