Skip to content

Conversation

@kylebuch8
Copy link
Contributor

@kylebuch8 kylebuch8 commented Sep 9, 2022

Converting pfe-datetime to pfe-timestamp.

Related issues

Preview

Link(s) to demo page(s) where this element can be viewed:

What has changed and why

Converting pfe-datetime to pfe-timestamp.

Browser requirements

Your component should work in all of the following environments:

  • Latest 2 versions of Edge
  • Latest 2 versions of Firefox (one on Mac OS, one of Windows OS)
  • Firefox 78 (or latest version for Red Hat Enterprise Linux distribution)
  • Latest 2 versions of Chrome (one on Mac OS, one of Windows OS)
  • Latest 2 versions of Safari
  • Android mobile device (such as the Galaxy S9)
  • Apple mobile device (such as the iPhone X)
  • Apple tablet device (such as the iPhone Pro)

Ready-for-merge Checklist

  • Expected files: all files in this pull request are related to one request or issue (no stragglers or scope-creep).
  • Tests have been updated to cover these changes.
  • Browser testing passed.
  • Changelog updated.
  • Documentation (README.md, WHY.md, etc.) updated or added.
  • Link to the demo recording:
  • Approved by designer.

Merging

Please squash when merging and ensure your commit message uses conventional commit formatting.

Be sure to share your updates with the [email protected] mailing list!

@changeset-bot
Copy link

changeset-bot bot commented Sep 9, 2022

🦋 Changeset detected

Latest commit: 8f725c7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@patternfly/pfe-tooltip Minor
@patternfly/pfe-timestamp Minor
@patternfly/pfe-tools Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added work in progress POC / Not ready for review demo Updating demo pages docs Documentation updates functionality Functionality, typically pertaining to the JavaScript. styles An issue or PR pertaining only to CSS/Sass tests Related to testing tools Development and build tools AT passed Automated testing has passed labels Sep 9, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2022

Deploy Preview for patternfly-elements ready!

Name Link
🔨 Latest commit f7ee0f1
😎 Deploy Preview https://deploy-preview-2147--patternfly-elements.netlify.app/

To edit notification comments on pull requests, go to your Netlify site settings.

@kylebuch8
Copy link
Contributor Author

One issue we're going to need to work through is adding a dashed underline to text when the pfe-timestamp component is surrounded by pfe-tooltip. I have an idea on the approach but I'm not sure if I like it.

connectedCallback() {
  super.connectedCallback();
  
  if (this.parentElement && this.parentElement.tagName.toLowerCase() === 'pfe-tooltip') {
    this.classList.add("has-tooltip");
  }
}

Something like that.

@bennypowers
Copy link
Member

One issue we're going to need to work through is adding a dashed underline to text when the pfe-timestamp component is surrounded by pfe-tooltip. I have an idea on the approach but I'm not sure if I like it.

connectedCallback() {
  super.connectedCallback();
  
  if (this.parentElement && this.parentElement.tagName.toLowerCase() === 'pfe-tooltip') {
    this.classList.add("has-tooltip");
  }
}

Something like that.

how about this in pfe-tooltip styles:

:host {
  --_timestamp-text-decoration: underline dashed 1px 4px;
}

@kylebuch8 kylebuch8 marked this pull request as ready for review September 28, 2022 15:17
Copy link
Member

@bennypowers bennypowers left a comment

Choose a reason for hiding this comment

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

tiny changes otherwise g2g

@heyMP
Copy link
Contributor

heyMP commented Oct 11, 2022

how about this in pfe-tooltip styles:

:host {
  --_timestamp-text-decoration: underline dashed 1px 4px;
}

To prevent altering "private" variable names from outside of the component I wonder if we can use the magical space toggles trick 😀

⚠️ WARNING: EXTREME PSUEDO CODE AHEAD

:host {
  --pfe-tooltip-enabled: ;
}

@bennypowers
Copy link
Member

this is very cool and also very confusing and i'm having a hard time finding the benefit delta between sprinkling media queries throughout a stylesheet and sprinkling comments with links to a blog post in all the same spots

@heyMP
Copy link
Contributor

heyMP commented Oct 11, 2022

@bennypowers yeah I agree. Something to keep in the back of the mind.

Copy link
Member

@bennypowers bennypowers left a comment

Choose a reason for hiding this comment

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

couple small changes please then we're g2g (pending #2198)

@bennypowers bennypowers enabled auto-merge (squash) October 25, 2022 15:57
@bennypowers bennypowers merged commit daba8a5 into main Oct 25, 2022
@bennypowers bennypowers deleted the feat/pfe-timestamp-1-to-1 branch October 25, 2022 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AT passed Automated testing has passed demo Updating demo pages docs Documentation updates functionality Functionality, typically pertaining to the JavaScript. styles An issue or PR pertaining only to CSS/Sass tests Related to testing tools Development and build tools work in progress POC / Not ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants