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

feat(field): added character meter #2091

Closed
wants to merge 2 commits into from

Conversation

ArtBlue
Copy link
Contributor

@ArtBlue ArtBlue commented Jul 7, 2023

Fixes #2047

  • This PR contains CSS changes
  • This PR does not contain CSS changes

Description

Field description (or error) and character meter was added as a grouped pair. Since positioning of one in this type of layout would impact the other, they needed to be within the same container. As such, minor markup modifications were needed to accomplish that. I don't think this is breaking since the new character meter feature can bear the responsibility of switching the layout to this new grid-based layout.

The docs also include the interactive portion of this effort that should serve as a model for how ebay ui core handles DOM updates. A debounced keystrokes approach was used limit DOM updates and screen reader read-outs.

Notes

The resolution of this issue depends on the approval of an a11y review.

Screenshots

image

Checklist

  • I verify the build is in a non-broken state
  • I verify all changes are within scope of the linked issue
  • I regenerated all CSS files under dist folder
  • I tested the UI in all supported browsers
  • I did a visual regression check of the components impacted by doing a Percy build and approved the build
  • I tested the UI in dark mode and RTL mode
  • I added/updated/removed Storybook coverage as appropriate

@ArtBlue ArtBlue self-assigned this Jul 7, 2023
Copy link
Member

@LuLaValva LuLaValva left a comment

Choose a reason for hiding this comment

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

I left one comment on your implementation, but before I go any further I'd like to ensure that this is the right general direction; are you sure we need to introduce field--grid to get this working? We already have field--table to handle a lot of the general layout, so this sort of feels redundant to me. Ian's original suggestion was to add a modifier or a wrapper around field__description which allows for multiple cells, I'd like to know the reason why you didn't go with this path.

If you do decide to stick with field--grid it looks like you also need to figure out a way to deal with long descriptions with text that needs to wrap, I don't believe that is currently accounted for.

<div id="field-character-meter-1-metergroup" class="field__character-meter">
<meter aria-label="Characters Remaining" class="field__character-meter_meter clipped" high="100" low="30" min="0" max="140" value="0"></meter>

<div id="field-character-meter-1" class="field__character-meter_text">
Copy link
Member

Choose a reason for hiding this comment

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

Is id="field-character-meter-1" used at all for accessibility? I can see where the others are referenced, but not this one.

@ArtBlue
Copy link
Contributor Author

ArtBlue commented Jul 10, 2023

I left one comment on your implementation, but before I go any further I'd like to ensure that this is the right general direction; are you sure we need to introduce field--grid to get this working? We already have field--table to handle a lot of the general layout, so this sort of feels redundant to me.

I originally, started implementing it using table. Soon thereafter, I came up against a layout roadblock. I decided to change course for a few reasons-

  1. I found a simpler way to accommodate layout adjustments using grid that got around the specific roadblock.
  2. By Ian's admission, having to use empty cells was "gross." These sorts of artifacts are easily avoidable in grid.
  3. CSS grid is IMO a more potent and flexible way of doing layouts like these, especially where things can shift so dramatically based on modifiers.

I would really like to slowly move the table layout of all field layouts to grid.

Ian's original suggestion was to add a modifier or a wrapper around field__description which allows for multiple cells, I'd like to know the reason why you didn't go with this path.

In essence, this approach follows Ian's suggestion of "expand on the current field__description to allow left and right portions." I did stray away from using the current classes to avoid potential style collisions and because a field_description would not be accurately named anymore since it is now housing the true field description (helper text) and the not so true character meter. I suppose it might be semantics, but it might be extremely confusing for devs to see the same name pointing to totally different types of things.

If you do decide to stick with field--grid it looks like you also need to figure out a way to deal with long descriptions with text that needs to wrap, I don't believe that is currently accounted for.

I did make additional layout tweaks that now might allow for the use of table layouts. I would still prefer to stick with grid rather than go back to re-explore table. I'm not totally opposed to it, but since grid is superior, it's my preference to go ahead with it.

I did verify that longer descriptions were working properly. If there is a specific issue you're seeing, please post a screenshot and let me know where.

@LuLaValva
Copy link
Member

LuLaValva commented Jul 10, 2023

Ok, I understand why it makes sense to add grid but I don't like it as a requirement for the character meter since I think the same can be accomplished with much less styling. Here's a mocked example of what I was thinking based on Ian's suggestion:

image

What I like about the flexbox approach is that it could be used in both table and grid formats.

I did verify that longer descriptions were working properly. If there is a specific issue you're seeing, please post a screenshot and let me know where.

I just did a quick test where I added a few more words to the first description, and it pushed everything to the right instead of wrapping text.

image

@ArtBlue
Copy link
Contributor Author

ArtBlue commented Jul 10, 2023

Ok, I understand why it makes sense to add grid but I don't like it as a requirement for the character meter since I think the same can be accomplished with much less styling. Here's a mocked example of what I was thinking based on Ian's suggestion:

image What I like about the flexbox approach is that it could be used in _both_ table and grid formats.

I did verify that longer descriptions were working properly. If there is a specific issue you're seeing, please post a screenshot and let me know where.

I just did a quick test where I added a few more words to the first description, and it pushed everything to the right instead of wrapping text.

image

I'm not sure what you were referring to with the flex example. A bit confused there. That text helper element already has a flex.

Thanks for the screenshot of the description text. I thought I had that taken care of; I'll take a look.

As far as the table/grid approach...Perhaps the same might be able to be accomplished with less styling. That remains to be seen based on all the requirements. However, you're going to need more markup, actually a lot more markup - empty cells, rows, etc. I would ultimately prefer more styling over more markup.

I'm not necessarily opposed to it, but the tradeoffs are not overwhelming. If it solves the UI layouts we need more exquisitely, let's go for it. Do you have the full switch to table layouts working locally that addresses all use cases?

@agliga
Copy link
Contributor

agliga commented Jul 12, 2023

Actually I thought of the same thing as Luke, we don't really need the grid layout imo. Grid should be used when there are multiple columns/rows and different combinations of them. In this it seems like that is not the case, so flex should be fine to handle this.

The first thing is we need to verify with design as well when we use the character count. My understanding is that it is primarily used with floating label or the label on top. From Figma I don't see any usecases with the label on the left (I believe that is a legacy usecase anyways)

My understaind is that design is trying to phase out the label on the left, but we need to check. If it is being phased out, then it we can do it easy without grid since we can do a flex layout with a column direction. We should only support new layouts with the character count and not legacy ones imo. (This would be a good motivation to have teams to remove the old layouts as well)

I think the approach is good and it works well. But if it can be simpler, I'm all for it as well.

@agliga agliga mentioned this pull request Jul 14, 2023
9 tasks
@agliga
Copy link
Contributor

agliga commented Jul 19, 2023

Closing PR. Will reopen if we need to add meter.

@agliga agliga closed this Jul 19, 2023
@agliga agliga added this to the 16.5.0 milestone Jul 20, 2023
@agliga agliga deleted the 2047-field-add-helper-text-sections branch April 30, 2024 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants